Edu logit lens#114
Open
skysneakers wants to merge 17 commits into
Open
Conversation
Patching now slice-assigns on model.layers_output[i] only — same pattern as nnsightful.tools.activation_patching — instead of also reassigning model.layers[i].output, which was redundant and could leave project_on_vocab(hs) reading the pre-patch value. Inlines the logit-lens formatter because LogitLensTool doesn't override _format; the prior call to logit_lens_tool._format(...) fell through to Tool._format in nnsightful._base, returning None. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
@skysneakers is attempting to deploy a commit to the NDIF Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 Walkthrough<review_stack_artifact_start /> All changes implementing Logit Lens Intro and Causal Mediation Intro: backend router and handlers, web API hooks, chart/type additions, UI components/pages/layouts, navigation/sidebar wiring, dependency and build config, and typing/test tweaks.Startup script change and FastAPI app environment loading; register causal_mediation router and add global exception handler.range_7a38431477fa range_318679e0b2e9 range_b6568041c4e5 range_87460c4c5ec1New FastAPI router module: request/response Pydantic models, _format_lens, _run_causal_mediation, and /start and /results handlers formatting logits into LogitLensData.range_7cb6ebb64bb9 range_438d80a4d48b range_1d1bb487b04f range_ae90557cfa2c range_3822f1b35247Add top-level package.json and update API start command to run uvicorn via python -m uvicorn.range_a46bc8fa9394 range_7a38431477faAdd edulogitlens dependency, include in transpilePackages, and add Tailwind content paths for the package.range_8dd9bb173b45 range_5163351294ac range_14eb1289dde7 range_8dd9bb173b45Introduce types/modules for LogitLensIntro and CMIntro, extend ChartType/ToolType unions, and add causal mediation endpoints to config.range_f13cf5417cf5 range_47ee7ce387a8 range_5c2f1dbc3ce7 range_73c3033db74b range_73c3033db74bAdd createLogitLensIntroChartPair and createCMIntroChartPair and import usage in chartApi.range_dc6997fac0b6 range_c7f0e579ad01 range_dc6997fac0b6Add getLogitLensIntro helper and useLogitLensIntro mutation with optimistic cache behavior and polling.range_8af1fdde224f range_abb4b22bba48 range_8af1fdde224fAdd CM Intro API module with CMIntroLensRequest/Result/Intervention types, runLogitLens helper, useCMIntroLogitLens and useCMIntroIntervention mutations that persist chart data and merge interventions.range_9b4f9cfb56d5 range_cdb80d78f961 range_996213c54cbb range_a93257c11783Update chartApi to import new create*ChartPair functions, add useCreateLogitLensIntroChartPair/useCreateCMIntroChartPair, remove deprecated hooks, and narrow optimistic cache typings.range_f9d4eb72b167 range_0f87ff987bab range_e87fa702a08d range_e87fa702a08d range_e87fa702a08dAdd LogitLensIntroArea, LogitLensIntroControls, LogitLensIntroDisplay, layout and page; includes tokenization, controls (top-k, entropy), run flow, transform to edulogitlens format, and dev mock page.range_73b30a63862f range_71d54d3565d1 range_38dbb4979f3d range_2278326dbc82 range_5c77083d0ba6 range_533612d6d362 range_95c59c4494ca range_ac31400694e2 range_c5d4d95f11f5 range_270c103a872e range_ffd631acf587 range_808f4cf72975 range_84fca1e51b89 range_41c966d26562 range_24fb8256a32d range_6c75d2c47321 range_8ca336ec53cd range_2b4df1445b87 range_103df08b0c3a range_c6dc475f329f range_fb9cba3aca6a range_1a483ab07f32 range_94ab02f0044d range_657df509f18dAdd CMIntroArea, CMIntroDisplay, CMIntroChartPage, layout, tutorial steps, and tutorial Zustand store; implement hydration, autosave, intervention handling and transform to explorer format.range_2a2aff91f1f5 range_ebad2a339e58 range_655c56a0b7e0 range_786588c7b5fe range_70f06e5d0774 range_2d32611a6bad range_fbe85b600505 range_d03ac604a371 range_337abb96d6d8 range_d6a67bfe911a range_a231f382367e range_9bcc8910cdc4 range_386db4fbf270 range_7cfd048b8943 range_ee2ea933ecfc range_daac487ebd51 range_054ed11187f2 range_4ad2a88476d7 range_ea7d4fd42e14Add sidebar create buttons and collapsed icons for new chart types, wiring creation mutations and navigation to dedicated routes; update ChartCard badge rendering and workspace page redirects.range_2c4565c31ab3 range_3f62e3f5ed6e range_26e81c9147a8 range_7ec2c7e8dc24 range_ea7f8ac04e7a range_c13a8e299c90 range_14301ce3e7cd range_079f424d283e range_e03b08f39e47 range_64cbafe50cf9 range_28d1a2472e92 range_fc3852904309 range_b76ca21a7849Tighten TypeScript types (utils, documentQueries, supabase mock), D3 selection typings, Heatmap/ChartDisplay predicates, k6 test typing, instrumentation dynamic import, TourProvider UI changes, and test cast updates.range_b053572b766a range_9a8149f5c050 range_426028bae69f range_6df94d65992b range_1de0ab770a7f range_bd24f36fa4bd range_6e03dd8abcbc range_aa97b56aecce range_c65254852961 range_24154615a029 range_5f050e256dbb range_09b2bab770f1 range_c48c55fa10ee range_454c915c92d0 range_6c25afcd9939 range_3a3365a3d714 range_e3ac8aead39f range_3615057ec081 range_76a6b42f9d60 range_24fda29fe860 range_199326a9e130 range_156eebe47a79 range_2884a21dc167 range_debc195a2ab5 range_735decdae8d1 range_02025f01ae60 range_ae81c990f31f range_c29bd0b3d7a4 range_5f787fe245fa range_a4dc65bdf106<unassigned_ranges> 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
workbench/_web/src/app/workbench/[workspaceId]/components/ChartCardsSidebar.tsx (1)
163-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRoute resolution should not depend only on
toolType.This helper can misroute when
toolTypeis missing butchartTypeis set (e.g., after delete navigation), falling back to/workbench/${workspaceId}/${chartId}instead of the typed route.Suggested fix
- const navigateToChart = (chartId: string, toolType?: string) => { + const navigateToChart = ( + chartId: string, + toolType?: string, + chartType?: ChartMetadata["chartType"], + ) => { + const type = toolType ?? chartType; // Route based on tool type - if (toolType === "lens2") { + if (type === "lens2") { router.push(`/workbench/${workspaceId}/lens2/${chartId}`); - } else if (toolType === "activation-patching") { + } else if (type === "activation-patching") { router.push(`/workbench/${workspaceId}/activation-patching/${chartId}`); - } else if (toolType === "logit-lens-intro") { + } else if (type === "logit-lens-intro") { router.push(`/workbench/${workspaceId}/logit-lens-intro/${chartId}`); - } else if (toolType === "cm-intro") { + } else if (type === "cm-intro") { router.push(`/workbench/${workspaceId}/cm-intro/${chartId}`); } else { router.push(`/workbench/${workspaceId}/${chartId}`); } };- if (nextChart) navigateToChart(nextChart.id, nextChart.toolType ?? undefined); + if (nextChart) navigateToChart(nextChart.id, nextChart.toolType ?? undefined, nextChart.chartType);🤖 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 `@workbench/_web/src/app/workbench/`[workspaceId]/components/ChartCardsSidebar.tsx around lines 163 - 175, navigateToChart currently decides routes only by toolType, causing misroutes when toolType is undefined but chartType is available (e.g., after delete); update navigateToChart to fall back to chartType when toolType is falsy: compute an effectiveType = toolType || chartType and use that to select the route branches (check for "lens2", "activation-patching", "logit-lens-intro", "cm-intro") and otherwise use the default router.push(`/workbench/${workspaceId}/${chartId}`); touch the navigateToChart function and ensure references to router.push and workspaceId remain unchanged.workbench/_web/src/app/workbench/[workspaceId]/logit-lens-intro/[chartId]/page.tsx (1)
1-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the Prettier check failure before merge.
The PR currently fails formatting checks; please run Prettier on the changed files and commit the result so CI can pass.
🤖 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 `@workbench/_web/src/app/workbench/`[workspaceId]/logit-lens-intro/[chartId]/page.tsx around lines 1 - 53, The file fails Prettier formatting; run Prettier on the edited file(s) and commit the formatted output so CI passes — e.g., run your project's formatter script (npm/yarn pnpm run format or prettier --write .), or format just the changed file that contains the LogitLensIntroChartPage component, then stage and commit the changes; ensure the export default function LogitLensIntroChartPage and its imports/JSX are updated by the formatter before pushing.
🤖 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 `@package.json`:
- Line 1: package.json is not prettified; run the project's Prettier formatting
on the root manifest (package.json) to restore proper JSON formatting according
to the repository Prettier config so the CI formatting check passes—apply
Prettier to package.json and re-commit the updated file.
In `@workbench/_api/main.py`:
- Around line 70-77: The global_exception_handler currently logs the full
traceback and returns it to clients; change it so logging.error still records
the full traceback (use traceback.format_exception as now) but the JSONResponse
from global_exception_handler returns only a generic error message (e.g.
{"detail":"Internal server error"}) and no traceback or internal exception text;
update the handler around the logging.error and return JSONResponse
(status_code=500) with a generic message and do not include exc, tb, or
exc.__traceback__ in the response body while keeping the existing logging call
in global_exception_handler.
In `@workbench/_api/routes/causal_mediation.py`:
- Around line 18-27: Validate user-controlled indices and topk before any tensor
indexing: add Pydantic validators on CausalMediationRequest for src_layer,
tgt_layer, src_token_pos, tgt_token_pos, and topk to ensure they are integers,
non-negative, and within sensible bounds (e.g., topk >=1 and <= a safe max),
and/or perform explicit checks at the start of the request handler that compute
tensors (the places using these fields around the code regions referenced) and
raise a 400 HTTP error with a clear message for invalid values; ensure checks
compare layer indices against the model's available layer count and token
positions against sequence length (or defer validation until you have the tensor
shapes), and clamp or reject out-of-range topk against the axis size before any
torch/numpy indexing or topk ops to prevent runtime exceptions.
- Around line 64-67: The call logits.to("cpu") is a no-op because .to() returns
a new tensor; assign the result or, as suggested, move the actual tensor used
later: replace the standalone logits.to("cpu") with either logits =
logits.to("cpu") or, preferably, move probs instead by assigning probs =
probs.to("cpu") after computing probs = torch.nn.functional.softmax(logits,
dim=-1), and then delete or let logits go out of scope (e.g., del logits) to
free GPU memory before calling torch.topk on probs to compute top_indices.
In `@workbench/_web/package.json`:
- Around line 55-56: Update the React dependencies in package.json to react and
react-dom version 19 to satisfy Next.js 15 App Router requirements: locate the
dependency entries for "react" and "react-dom" (the same area as "next":
"15.5.9") and bump their versions to the appropriate stable 19.x release, then
regenerate the lockfile (npm/yarn/pnpm install), run type checks and app tests,
and fix any type or hydration errors that surface from the React major upgrade.
In
`@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/components/CMIntroArea.tsx:
- Around line 182-185: The current configModelUnavailable only checks
srcTokenizedModel and misses stale target tokenization; update the condition to
include tgtTokenizedModel so either mismatch flags the warning. Change the
assignment of configModelUnavailable (currently referencing srcTokenizedModel,
selectedModel) to check both (srcTokenizedModel !== selectedModel) ||
(tgtTokenizedModel !== selectedModel) and return the mismatched model identifier
(e.g., srcTokenizedModel if it mismatches, else tgtTokenizedModel) or null if
neither mismatch; refer to configModelUnavailable, srcTokenizedModel,
tgtTokenizedModel, and selectedModel when making this change.
In
`@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/components/CMIntroDisplay.tsx:
- Around line 102-109: persistedResultData (computed in the useMemo) is
overriding in-session interventions when persistedData.result exists; change the
logic so persistedResultData is only used during initial hydration or is cleared
when inputs change: detect the hydration/init state or watch the prompt/lens
input values (the deps for the memo or a separate effect), and only return
transformToEduFormat(persistedData.result) when still in the initial load
context (or else return undefined), or add an effect to set persistedResultData
to undefined whenever prompt/lens inputs change so CausalMediationExplorer
receives the current session result instead of stale persisted output.
In `@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/page.tsx:
- Around line 76-98: The debounced autosave in the useEffect can persist stale
prompts because overlapping async saves (started by getChartById ->
setChartData) may complete out of order; add a monotonic request guard using a
ref (e.g., saveSeqRef) that you increment before launching the async handler,
capture the current seq in the handler, and before calling
setChartData/invalidateQueries check that the captured seq matches
saveSeqRef.current so only the latest save writes; update references in this
effect (useEffect, getChartById, setChartData, PROMPT_AUTOSAVE_DEBOUNCE_MS,
queryClient.invalidateQueries, queryKeys.charts.chart) and ensure the guard is
used both before writing and when deciding to return early to avoid stale
overwrites.
In
`@workbench/_web/src/app/workbench/`[workspaceId]/components/ChartCardsSidebar.tsx:
- Around line 70-71: Prettier formatting errors were introduced around the
useCreateLogitLensIntroChartPair and useCreateCMIntroChartPair hooks; run
Prettier on
workbench/_web/src/app/workbench/[workspaceId]/components/ChartCardsSidebar.tsx
(or apply your editor's auto-format) to fix whitespace/line breaks around the
declarations for createLogitLensIntroPair/isCreatingLogitLensIntro and
createCMIntroPair/isCreatingCMIntro (also check the same for the occurrence near
line ~182) and commit the reformatted file so CI passes.
In
`@workbench/_web/src/app/workbench/`[workspaceId]/logit-lens-intro/[chartId]/components/LogitLensIntroArea.tsx:
- Around line 46-63: The useEffect that reads introConfig.data?.model doesn't
react to changes in the model string when config.id remains the same; update the
effect dependencies so it includes the config's model value (e.g.
config?.data?.model or config) in addition to models and setSelectedModelIdx,
and ensure you still reference setConfigModelUnavailable inside the dependency
list so the selectedModelIdx and configModelUnavailable state are resynced
whenever the config's model field changes.
In
`@workbench/_web/src/app/workbench/`[workspaceId]/logit-lens-intro/[chartId]/components/LogitLensIntroControls.tsx:
- Around line 283-292: The token container div referenced by tokenContainerRef
in LogitLensIntroControls (which uses isExecuting and calls escapeTokenArea on
click) is not keyboard-reachable; replace the plain div with a semantic,
keyboard-focusable control (e.g., a button or a div with role="button",
tabIndex={0}, and keyboard handlers) so keyboard users can trigger
escapeTokenArea: ensure the element still respects the isExecuting state
(disabled or cursor styles) and add an onKeyDown handler that calls
escapeTokenArea on Enter/Space to mirror the onClick behavior and preserve
existing className/cursor logic.
- Around line 266-281: The Prompt label is not programmatically linked to the
textarea; update the Label and Textarea so they are associated by adding a
matching identifier: give the Textarea an id (e.g., "prompt-textarea" or a
generated id using chartId/workspaceId) and set the Label's htmlFor to that id.
If Textarea is a custom component, ensure it accepts/forwards the id prop to the
underlying <textarea> element (check the Textarea component implementation) so
the Label (Label component) correctly targets the editable element referenced by
textareaRef and used in editingText/handlePromptChange/autoResizeTextarea.
- Around line 200-215: The submit flow calls computeLens(...) and
updateConfig(...) without handling rejections; wrap the sequential calls (the
computeLens call that uses lensRequest with completion=config and chartId,
followed by updateConfig with configId=initialConfig.id and config payload
including data=config, workspaceId, type="logit-lens-intro") in a try/catch,
surface any error to the user (e.g., set a local error state or call the
existing toast/notification helper) and ensure any in-progress UI state is
cleared on failure; also consider reverting optimistic UI changes if any were
applied prior to calling computeLens/updateConfig so initialConfig remains
consistent.
In
`@workbench/_web/src/app/workbench/`[workspaceId]/logit-lens-intro/[chartId]/components/LogitLensIntroDisplay.tsx:
- Around line 127-141: The loading containers in LogitLensIntroDisplay (the JSX
returned inside the `if (isLoading)` and `if (isRunning)` branches) must be
marked as polite live regions so assistive tech gets updates; update those
top-level divs to include aria-live="polite" (and optionally role="status" for
clearer semantics) while keeping the existing loader and text intact so the
message "Computing logit lens visualization..." and the spinner are announced.
- Around line 144-151: The component currently falls back to mockData and
renders LogitLensGrid when real chart data is missing; change the logic in
LogitLensIntroDisplay so it never shows mockData on the real chart route:
compute eduData only when hasData is true (use
transformToEduFormat(introChart.data!) only inside the hasData branch) and, when
hasData is false or transform returns falsy, render a clear empty/error state
(e.g., a placeholder message or nothing) instead of <LogitLensGrid
data={mockData} />; update any references to eduData accordingly to avoid
accidentally passing mockData into LogitLensGrid.
In `@workbench/_web/src/lib/api/chartApi.ts`:
- Around line 297-312: The onSuccess handlers for the create mutations currently
call queryClient.invalidateQueries({ queryKey: queryKeys.charts.sidebar() })
without the workspaceId, so update both create hooks (the earlier create
useMutation shown and useCreateCMIntroChartPair) to invalidate the
workspace-scoped cache by passing the created mutation's workspaceId into the
query key (use the onSuccess signature that accepts (_, variables) or (_data,
variables) and call queryClient.invalidateQueries({ queryKey:
queryKeys.charts.sidebar({ workspaceId: variables.workspaceId }) })). Ensure you
reference queryKeys.charts.sidebar and the mutation variable name workspaceId
used in the mutationFn.
In `@workbench/_web/src/lib/api/cmIntroApi.ts`:
- Around line 61-158: The hook-only flows need standalone exported async
executors so callers can run the operations without the hook's toast/error
wiring; extract the core logic from mutationFn into two exported functions (e.g.
export async function runCMIntroLogitLens(request: CMIntroLensRequest) :
Promise<CMIntroLensResult> and export async function
runCMIntroIntervention(request: CMIntroInterventionRequest) :
Promise<LogitLensIntroData>), move the header creation,
runLogitLens/startAndPoll calls, chart merging/ setChartData and return values
into those functions, then have useCMIntroLogitLens's and
useCMIntroIntervention's mutationFn simply call the corresponding exported
runner and return its result; keep existing toast/error handling and
queryClient.invalidateQueries in the hooks.
In `@workbench/_web/src/lib/api/logitLensIntroApi.ts`:
- Around line 19-35: Export the bare async function getLogitLensIntro so callers
can use it without the mutation-hook toast behavior: locate the
getLogitLensIntro function and add an export (named export) for it, ensuring the
existing implementation that builds request (using lensRequest.completion
fields), calls createUserHeadersAction, and uses startAndPoll with
config.endpoints.startLens2 and config.endpoints.resultsLens2 remains unchanged;
keep the mutation hook that wraps this function separate so callers may import
either the hook or the exported getLogitLensIntro directly.
- Around line 82-87: The code is invalidating the sidebar cache using a
hardcoded key ["chartsForSidebar", chart.workspaceId]; replace that with the
centralized query key factory to avoid drift. Update the first
queryClient.invalidateQueries call to use the canonical factory (e.g.
queryKeys.charts.forSidebar(chart.workspaceId)) instead of the literal array,
keeping the second invalidateQueries call that uses
queryKeys.charts.configByChart(variables.lensRequest.chartId) unchanged;
reference queryClient.invalidateQueries, chart.workspaceId,
queryKeys.charts.forSidebar (or the actual factory name in queryKeys.ts) and
queryKeys.charts.configByChart when making the change.
In `@workbench/_web/src/types/charts.ts`:
- Around line 70-75: Chart config union ConfigData is missing the cm-intro
payload type, causing a typed hole; add a dedicated config type for the cm-intro
chart (e.g., CmIntroConfigData or a re-exported nnsightful type following the
project naming convention) and include it in the ConfigData union alongside
LensConfigData, Lens2ConfigData, PatchingConfig, ActivationPatchingConfigData,
and LogitLensIntroConfigData; ensure the new type matches the actual cm-intro
payload shape and is exported with the workbench-native name so
ChartType/ToolType handling is fully typed.
---
Outside diff comments:
In
`@workbench/_web/src/app/workbench/`[workspaceId]/components/ChartCardsSidebar.tsx:
- Around line 163-175: navigateToChart currently decides routes only by
toolType, causing misroutes when toolType is undefined but chartType is
available (e.g., after delete); update navigateToChart to fall back to chartType
when toolType is falsy: compute an effectiveType = toolType || chartType and use
that to select the route branches (check for "lens2", "activation-patching",
"logit-lens-intro", "cm-intro") and otherwise use the default
router.push(`/workbench/${workspaceId}/${chartId}`); touch the navigateToChart
function and ensure references to router.push and workspaceId remain unchanged.
In
`@workbench/_web/src/app/workbench/`[workspaceId]/logit-lens-intro/[chartId]/page.tsx:
- Around line 1-53: The file fails Prettier formatting; run Prettier on the
edited file(s) and commit the formatted output so CI passes — e.g., run your
project's formatter script (npm/yarn pnpm run format or prettier --write .), or
format just the changed file that contains the LogitLensIntroChartPage
component, then stage and commit the changes; ensure the export default function
LogitLensIntroChartPage and its imports/JSX are updated by the formatter before
pushing.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bab83ebd-c71a-475e-b35e-16513a5b7371
⛔ Files ignored due to path filters (2)
workbench/_web/bun.lockis excluded by!**/*.lockworkbench/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
package.jsonscripts/api.shworkbench/_api/main.pyworkbench/_api/routes/__init__.pyworkbench/_api/routes/causal_mediation.pyworkbench/_web/next.config.jsworkbench/_web/package.jsonworkbench/_web/src/app/dev/logit-lens-intro/page.tsxworkbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/components/CMIntroArea.tsxworkbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/components/CMIntroDisplay.tsxworkbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/layout.tsxworkbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/page.tsxworkbench/_web/src/app/workbench/[workspaceId]/components/ChartCard.tsxworkbench/_web/src/app/workbench/[workspaceId]/components/ChartCardsSidebar.tsxworkbench/_web/src/app/workbench/[workspaceId]/logit-lens-intro/[chartId]/components/LogitLensIntroArea.tsxworkbench/_web/src/app/workbench/[workspaceId]/logit-lens-intro/[chartId]/components/LogitLensIntroControls.tsxworkbench/_web/src/app/workbench/[workspaceId]/logit-lens-intro/[chartId]/components/LogitLensIntroDisplay.tsxworkbench/_web/src/app/workbench/[workspaceId]/logit-lens-intro/[chartId]/layout.tsxworkbench/_web/src/app/workbench/[workspaceId]/logit-lens-intro/[chartId]/page.tsxworkbench/_web/src/app/workbench/[workspaceId]/page.tsxworkbench/_web/src/lib/api/chartApi.tsworkbench/_web/src/lib/api/cmIntroApi.tsworkbench/_web/src/lib/api/logitLensIntroApi.tsworkbench/_web/src/lib/config.tsworkbench/_web/src/lib/queries/chartQueries.tsworkbench/_web/src/types/charts.tsworkbench/_web/src/types/cmIntro.tsworkbench/_web/src/types/logitLensIntro.tsworkbench/_web/tailwind.config.ts
| @@ -0,0 +1 @@ | |||
| {"dependencies": {}} No newline at end of file | |||
There was a problem hiding this comment.
Run Prettier on the root manifest.
This one-line JSON is likely the source of the current formatting warning in CI.
Proposed fix
-{"dependencies": {}}
+{
+ "dependencies": {}
+}📝 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.
Suggested change
| {"dependencies": {}} | |
| { | |
| "dependencies": {} | |
| } |
🧰 Tools
🪛 GitHub Actions: Run Checks / 0_Run Prettier.txt
[warning] 1-1: Prettier formatting check warning: code style issue found.
🪛 GitHub Actions: Run Checks / Run Prettier
[warning] Prettier reported a formatting warning.
🤖 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 `@package.json` at line 1, package.json is not prettified; run the project's
Prettier formatting on the root manifest (package.json) to restore proper JSON
formatting according to the repository Prettier config so the CI formatting
check passes—apply Prettier to package.json and re-commit the updated file.
Comment on lines
+70
to
+77
| @app.exception_handler(Exception) | ||
| async def global_exception_handler(request: Request, exc: Exception): | ||
| tb = traceback.format_exception(type(exc), exc, exc.__traceback__) | ||
| logging.error(f"Unhandled exception: {''.join(tb)}") | ||
| return JSONResponse( | ||
| status_code=500, | ||
| content={"detail": str(exc), "traceback": ''.join(tb[-3:])}, | ||
| ) |
There was a problem hiding this comment.
Do not return traceback details in production error responses.
Line 76 exposes internal exception content and stack trace to clients. That leaks implementation details and can expose sensitive data paths. Return a generic error message and keep full traceback only in server logs.
Suggested fix
`@app.exception_handler`(Exception)
async def global_exception_handler(request: Request, exc: Exception):
tb = traceback.format_exception(type(exc), exc, exc.__traceback__)
logging.error(f"Unhandled exception: {''.join(tb)}")
return JSONResponse(
status_code=500,
- content={"detail": str(exc), "traceback": ''.join(tb[-3:])},
+ content={"detail": "Internal server error"},
)🤖 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 `@workbench/_api/main.py` around lines 70 - 77, The global_exception_handler
currently logs the full traceback and returns it to clients; change it so
logging.error still records the full traceback (use traceback.format_exception
as now) but the JSONResponse from global_exception_handler returns only a
generic error message (e.g. {"detail":"Internal server error"}) and no traceback
or internal exception text; update the handler around the logging.error and
return JSONResponse (status_code=500) with a generic message and do not include
exc, tb, or exc.__traceback__ in the response body while keeping the existing
logging call in global_exception_handler.
Comment on lines
+18
to
+27
| class CausalMediationRequest(BaseModel): | ||
| model: str | ||
| src_prompt: str | ||
| tgt_prompt: str | ||
| src_token_pos: int | ||
| src_layer: int | ||
| tgt_token_pos: int | ||
| tgt_layer: int | ||
| topk: int = 5 | ||
| include_entropy: bool = True |
There was a problem hiding this comment.
Validate user-controlled indices and topk before tensor indexing.
src_layer, tgt_layer, token positions, and topk are used directly in indexing and topk ops. Invalid values will throw runtime errors and return 500s instead of clear 4xx validation errors.
Suggested fix
from fastapi import APIRouter, Depends
-from pydantic import BaseModel
+from pydantic import BaseModel, Field
+from fastapi import HTTPException
@@
class CausalMediationRequest(BaseModel):
@@
- src_token_pos: int
- src_layer: int
- tgt_token_pos: int
- tgt_layer: int
- topk: int = 5
+ src_token_pos: int = Field(ge=0)
+ src_layer: int = Field(ge=0)
+ tgt_token_pos: int = Field(ge=0)
+ tgt_layer: int = Field(ge=0)
+ topk: int = Field(default=5, ge=1)
@@
async def start_causal_mediation(
@@
):
model = state[req.model]
+ if req.src_layer >= model.num_layers or req.tgt_layer >= model.num_layers:
+ raise HTTPException(status_code=422, detail="Layer index out of range")Also applies to: 125-136, 181-183, 210-212
🤖 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 `@workbench/_api/routes/causal_mediation.py` around lines 18 - 27, Validate
user-controlled indices and topk before any tensor indexing: add Pydantic
validators on CausalMediationRequest for src_layer, tgt_layer, src_token_pos,
tgt_token_pos, and topk to ensure they are integers, non-negative, and within
sensible bounds (e.g., topk >=1 and <= a safe max), and/or perform explicit
checks at the start of the request handler that compute tensors (the places
using these fields around the code regions referenced) and raise a 400 HTTP
error with a clear message for invalid values; ensure checks compare layer
indices against the model's available layer count and token positions against
sequence length (or defer validation until you have the tensor shapes), and
clamp or reject out-of-range topk against the axis size before any torch/numpy
indexing or topk ops to prevent runtime exceptions.
Comment on lines
+64
to
+67
| probs = torch.nn.functional.softmax(logits, dim=-1) | ||
| logits.to("cpu") # free memory | ||
|
|
||
| _, top_indices = torch.topk(probs, k=top_k, dim=-1) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PyTorch, does tensor.to("cpu") mutate the tensor in place, or return a new tensor that must be assigned?
💡 Result:
In PyTorch, the tensor.to("cpu") method does not mutate the tensor in place [1]. Instead, it returns a new tensor that resides on the specified device [1][2]. Because the original tensor remains unchanged, you must assign the result of the operation to a variable if you wish to use the moved tensor [3][2]. For example: tensor = tensor.to("cpu") If the tensor is already on the target device and no copy is required, the method may return the original tensor itself, but it is still standard practice to assign the result to ensure your code handles all cases correctly [1].
Citations:
- 1: http://docs.pytorch.org/docs/main/generated/torch.Tensor.to.html
- 2: https://www.codecademy.com/resources/docs/pytorch/tensor-operations/moving-tensors-across-devices
- 3: https://stackoverflow.com/questions/63061779/pytorch-when-do-i-need-to-use-todevice-on-a-model-or-tensor
🏁 Script executed:
# Find and examine the file
fd "causal_mediation.py" --type fRepository: ndif-team/workbench
Length of output: 105
🏁 Script executed:
# Read the file around lines 64-67
cat -n workbench/_api/routes/causal_mediation.py | sed -n '55,80p'Repository: ndif-team/workbench
Length of output: 1033
🏁 Script executed:
# Check if logits is referenced after line 65
cat -n workbench/_api/routes/causal_mediation.py | sed -n '64,100p'Repository: ndif-team/workbench
Length of output: 1357
🏁 Script executed:
# Check the broader function context to see if logits is used elsewhere or has special handling
cat -n workbench/_api/routes/causal_mediation.py | sed -n '40,70p'Repository: ndif-team/workbench
Length of output: 1382
logits.to("cpu") is a no-op here.
Line 65 does not mutate logits; it returns a new tensor that must be assigned. Since logits is never used after this line, the intent to free memory is not achieved. If memory pressure is a concern, move probs instead, which is actually used in subsequent operations:
Suggested fix
- probs = torch.nn.functional.softmax(logits, dim=-1)
- logits.to("cpu") # free memory
+ probs = torch.nn.functional.softmax(logits, dim=-1).cpu()📝 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.
Suggested change
| probs = torch.nn.functional.softmax(logits, dim=-1) | |
| logits.to("cpu") # free memory | |
| _, top_indices = torch.topk(probs, k=top_k, dim=-1) | |
| probs = torch.nn.functional.softmax(logits, dim=-1).cpu() | |
| _, top_indices = torch.topk(probs, k=top_k, dim=-1) |
🤖 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 `@workbench/_api/routes/causal_mediation.py` around lines 64 - 67, The call
logits.to("cpu") is a no-op because .to() returns a new tensor; assign the
result or, as suggested, move the actual tensor used later: replace the
standalone logits.to("cpu") with either logits = logits.to("cpu") or,
preferably, move probs instead by assigning probs = probs.to("cpu") after
computing probs = torch.nn.functional.softmax(logits, dim=-1), and then delete
or let logits go out of scope (e.g., del logits) to free GPU memory before
calling torch.topk on probs to compute top_indices.
Comment on lines
+55
to
+56
| "next": "15.5.9", | ||
| "next-themes": "^0.4.6", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Next.js 15.5.x App Router projects, what React and react-dom versions are required/recommended? Is React 18 officially supported for App Router?
💡 Result:
For Next.js 15.x projects (including 15.5.x), the recommended and standard configuration is to use React 19 [1][2]. Regarding your specific questions: Required/Recommended Versions: Next.js 15 is designed to align with React 19 [1][2]. While Next.js 15 introduced support for React 19, it also provided backward compatibility for React 18 specifically for the Pages Router [1][2]. Is React 18 officially supported for App Router? No, React 18 is not officially supported for the App Router in Next.js 15 [1][2]. The App Router in Next.js 15 is built on React 19 (specifically utilizing React canary releases that include React 19 features) [1][3]. While you may technically be able to force older versions in some environments, the App Router relies on React 19's architecture—such as Server Components and new streaming APIs—and using React 18 with the App Router is not recommended and can lead to unpredictable behavior, hydration errors, and type inconsistencies [1][3]. If you are using the Pages Router, you have the flexibility to use either React 18 or React 19 [1][2]. However, for App Router projects, upgrading to React 19 is the standard requirement [1][2].
Citations:
🏁 Script executed:
cat -n workbench/_web/package.json | head -70Repository: ndif-team/workbench
Length of output: 2799
🏁 Script executed:
ls -la workbench/_web/ | grep -E "^d"Repository: ndif-team/workbench
Length of output: 405
🏁 Script executed:
ls -la workbench/_web/src/Repository: ndif-team/workbench
Length of output: 742
Upgrade React and react-dom to version 19 for Next.js 15 App Router compatibility.
Next.js 15 requires React 19 for App Router projects. React 18 is not officially supported for the App Router and will cause unpredictable behavior, hydration errors, and type inconsistencies due to missing React 19 features like Server Components and new streaming APIs.
Proposed fix
- "react": "^18.3.1",
- "react-dom": "^18.3.1",
+ "react": "^19.0.0",
+ "react-dom": "^19.0.0",Also applies to: 62-63
🧰 Tools
🪛 GitHub Actions: Run Checks / Run Prettier
[warning] Prettier reported a formatting warning.
🤖 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 `@workbench/_web/package.json` around lines 55 - 56, Update the React
dependencies in package.json to react and react-dom version 19 to satisfy
Next.js 15 App Router requirements: locate the dependency entries for "react"
and "react-dom" (the same area as "next": "15.5.9") and bump their versions to
the appropriate stable 19.x release, then regenerate the lockfile (npm/yarn/pnpm
install), run type checks and app tests, and fix any type or hydration errors
that surface from the React major upgrade.
Comment on lines
+61
to
+158
| export const useCMIntroLogitLens = () => { | ||
| const queryClient = useQueryClient(); | ||
|
|
||
| return useMutation({ | ||
| mutationKey: ["cmIntroLogitLens"], | ||
| mutationFn: async (request: CMIntroLensRequest): Promise<CMIntroLensResult> => { | ||
| const headers = await createUserHeadersAction(); | ||
| const topk = request.topk ?? 5; | ||
| const includeEntropy = request.includeEntropy ?? true; | ||
|
|
||
| const [source, target] = await Promise.all([ | ||
| runLogitLens(request.sourcePrompt, request.model, topk, includeEntropy, headers), | ||
| runLogitLens(request.targetPrompt, request.model, topk, includeEntropy, headers), | ||
| ]); | ||
|
|
||
| // Running the base lens invalidates any prior intervention/result, | ||
| // so we persist only the fresh { source, target } pair (alongside | ||
| // the prompts they were computed from, so revisits restore the UI). | ||
| // lastRunSourcePrompt/lastRunTargetPrompt snapshot the prompts that | ||
| // were actually run, so the predicted-next-token hint can hide on | ||
| // edits even though autosave keeps sourcePrompt/targetPrompt fresh. | ||
| const persisted: CMIntroChartData = { | ||
| sourcePrompt: request.sourcePrompt, | ||
| targetPrompt: request.targetPrompt, | ||
| lastRunSourcePrompt: request.sourcePrompt, | ||
| lastRunTargetPrompt: request.targetPrompt, | ||
| source, | ||
| target, | ||
| }; | ||
| await setChartData(request.chartId, persisted, "cm-intro"); | ||
|
|
||
| return { source, target }; | ||
| }, | ||
| onError: () => { | ||
| toast.error("Failed to run logit lens."); | ||
| }, | ||
| onSuccess: async (_data, variables) => { | ||
| const chartKey = queryKeys.charts.chart(variables.chartId); | ||
| await queryClient.invalidateQueries({ queryKey: chartKey }); | ||
| }, | ||
| }); | ||
| }; | ||
|
|
||
| export const useCMIntroIntervention = () => { | ||
| const queryClient = useQueryClient(); | ||
|
|
||
| return useMutation({ | ||
| mutationKey: ["cmIntroIntervention"], | ||
| mutationFn: async ( | ||
| request: CMIntroInterventionRequest, | ||
| ): Promise<LogitLensIntroData> => { | ||
| const headers = await createUserHeadersAction(); | ||
| const topk = request.topk ?? 5; | ||
| const includeEntropy = request.includeEntropy ?? true; | ||
|
|
||
| const body = { | ||
| model: request.model, | ||
| src_prompt: request.srcPrompt, | ||
| tgt_prompt: request.tgtPrompt, | ||
| src_token_pos: request.intervention.srcTokenPos, | ||
| src_layer: request.intervention.srcLayer, | ||
| tgt_token_pos: request.intervention.tgtTokenPos, | ||
| tgt_layer: request.intervention.tgtLayer, | ||
| topk, | ||
| include_entropy: includeEntropy, | ||
| }; | ||
|
|
||
| const result = await startAndPoll<LogitLensIntroData>( | ||
| config.endpoints.startCausalMediation, | ||
| body, | ||
| config.endpoints.resultsCausalMediation, | ||
| headers, | ||
| ); | ||
|
|
||
| // Merge onto existing chart data so we preserve source/target and | ||
| // the prompts they were computed from. | ||
| const existingChart = await getChartById(request.chartId); | ||
| const existingData = (existingChart?.data ?? {}) as Partial<CMIntroChartData>; | ||
| const merged: CMIntroChartData = { | ||
| ...existingData, | ||
| sourcePrompt: existingData.sourcePrompt ?? request.srcPrompt, | ||
| targetPrompt: existingData.targetPrompt ?? request.tgtPrompt, | ||
| intervention: request.intervention, | ||
| result, | ||
| }; | ||
| await setChartData(request.chartId, merged, "cm-intro"); | ||
|
|
||
| return result; | ||
| }, | ||
| onError: () => { | ||
| toast.error("Failed to run causal mediation intervention."); | ||
| }, | ||
| onSuccess: async (_data, variables) => { | ||
| const chartKey = queryKeys.charts.chart(variables.chartId); | ||
| await queryClient.invalidateQueries({ queryKey: chartKey }); | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Export bare async functions for both CM Intro operations.
Both flows are currently hook-only. Extract and export the async executors (logit-lens pair run, intervention run), then call them from mutationFn.
As per coding guidelines: "Export the bare async function alongside the mutation hook so features can call it without automatic toast error handling."
🧰 Tools
🪛 GitHub Actions: Run Checks / Run Prettier
[warning] Prettier reported a formatting warning.
🤖 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 `@workbench/_web/src/lib/api/cmIntroApi.ts` around lines 61 - 158, The
hook-only flows need standalone exported async executors so callers can run the
operations without the hook's toast/error wiring; extract the core logic from
mutationFn into two exported functions (e.g. export async function
runCMIntroLogitLens(request: CMIntroLensRequest) : Promise<CMIntroLensResult>
and export async function runCMIntroIntervention(request:
CMIntroInterventionRequest) : Promise<LogitLensIntroData>), move the header
creation, runLogitLens/startAndPoll calls, chart merging/ setChartData and
return values into those functions, then have useCMIntroLogitLens's and
useCMIntroIntervention's mutationFn simply call the corresponding exported
runner and return its result; keep existing toast/error handling and
queryClient.invalidateQueries in the hooks.
Comment on lines
+19
to
+35
| const getLogitLensIntro = async (lensRequest: LogitLensIntroRequest): Promise<LogitLensIntroData> => { | ||
| const headers = await createUserHeadersAction(); | ||
|
|
||
| const request = { | ||
| model: lensRequest.completion.model, | ||
| prompt: lensRequest.completion.prompt, | ||
| topk: lensRequest.completion.topk ?? 5, | ||
| include_entropy: lensRequest.completion.includeEntropy ?? true, | ||
| }; | ||
|
|
||
| return await startAndPoll<LogitLensIntroData>( | ||
| config.endpoints.startLens2, | ||
| request, | ||
| config.endpoints.resultsLens2, | ||
| headers, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Export the bare async API function.
getLogitLensIntro should be exported so callers can use it without mutation-hook toast behavior.
Proposed fix
-const getLogitLensIntro = async (lensRequest: LogitLensIntroRequest): Promise<LogitLensIntroData> => {
+export const getLogitLensIntro = async (lensRequest: LogitLensIntroRequest): Promise<LogitLensIntroData> => {As per coding guidelines: "Export the bare async function alongside the mutation hook so features can call it without automatic toast error handling."
📝 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.
Suggested change
| const getLogitLensIntro = async (lensRequest: LogitLensIntroRequest): Promise<LogitLensIntroData> => { | |
| const headers = await createUserHeadersAction(); | |
| const request = { | |
| model: lensRequest.completion.model, | |
| prompt: lensRequest.completion.prompt, | |
| topk: lensRequest.completion.topk ?? 5, | |
| include_entropy: lensRequest.completion.includeEntropy ?? true, | |
| }; | |
| return await startAndPoll<LogitLensIntroData>( | |
| config.endpoints.startLens2, | |
| request, | |
| config.endpoints.resultsLens2, | |
| headers, | |
| ); | |
| }; | |
| export const getLogitLensIntro = async (lensRequest: LogitLensIntroRequest): Promise<LogitLensIntroData> => { | |
| const headers = await createUserHeadersAction(); | |
| const request = { | |
| model: lensRequest.completion.model, | |
| prompt: lensRequest.completion.prompt, | |
| topk: lensRequest.completion.topk ?? 5, | |
| include_entropy: lensRequest.completion.includeEntropy ?? true, | |
| }; | |
| return await startAndPoll<LogitLensIntroData>( | |
| config.endpoints.startLens2, | |
| request, | |
| config.endpoints.resultsLens2, | |
| headers, | |
| ); | |
| }; |
🧰 Tools
🪛 GitHub Actions: Run Checks / Run Prettier
[warning] Prettier reported a formatting warning.
🤖 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 `@workbench/_web/src/lib/api/logitLensIntroApi.ts` around lines 19 - 35, Export
the bare async function getLogitLensIntro so callers can use it without the
mutation-hook toast behavior: locate the getLogitLensIntro function and add an
export (named export) for it, ensuring the existing implementation that builds
request (using lensRequest.completion fields), calls createUserHeadersAction,
and uses startAndPoll with config.endpoints.startLens2 and
config.endpoints.resultsLens2 remains unchanged; keep the mutation hook that
wraps this function separate so callers may import either the hook or the
exported getLogitLensIntro directly.
Comment on lines
+82
to
+87
| queryClient.invalidateQueries({ | ||
| queryKey: ["chartsForSidebar", chart.workspaceId], | ||
| }); | ||
| queryClient.invalidateQueries({ | ||
| queryKey: queryKeys.charts.configByChart(variables.lensRequest.chartId), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use centralized query keys for sidebar invalidation.
This segment hardcodes ["chartsForSidebar", chart.workspaceId] instead of the query key factory, which can drift from the canonical key.
Proposed fix
queryClient.invalidateQueries({
- queryKey: ["chartsForSidebar", chart.workspaceId],
+ queryKey: queryKeys.charts.sidebar(chart.workspaceId),
});As per coding guidelines: "Centralize React Query keys in src/lib/queryKeys.ts."
📝 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.
Suggested change
| queryClient.invalidateQueries({ | |
| queryKey: ["chartsForSidebar", chart.workspaceId], | |
| }); | |
| queryClient.invalidateQueries({ | |
| queryKey: queryKeys.charts.configByChart(variables.lensRequest.chartId), | |
| }); | |
| queryClient.invalidateQueries({ | |
| queryKey: queryKeys.charts.sidebar(chart.workspaceId), | |
| }); | |
| queryClient.invalidateQueries({ | |
| queryKey: queryKeys.charts.configByChart(variables.lensRequest.chartId), | |
| }); |
🧰 Tools
🪛 GitHub Actions: Run Checks / Run Prettier
[warning] Prettier reported a formatting warning.
🤖 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 `@workbench/_web/src/lib/api/logitLensIntroApi.ts` around lines 82 - 87, The
code is invalidating the sidebar cache using a hardcoded key
["chartsForSidebar", chart.workspaceId]; replace that with the centralized query
key factory to avoid drift. Update the first queryClient.invalidateQueries call
to use the canonical factory (e.g.
queryKeys.charts.forSidebar(chart.workspaceId)) instead of the literal array,
keeping the second invalidateQueries call that uses
queryKeys.charts.configByChart(variables.lensRequest.chartId) unchanged;
reference queryClient.invalidateQueries, chart.workspaceId,
queryKeys.charts.forSidebar (or the actual factory name in queryKeys.ts) and
queryKeys.charts.configByChart when making the change.
Comment on lines
70
to
+75
| export type ConfigData = | ||
| | LensConfigData | ||
| | Lens2ConfigData | ||
| | PatchingConfig | ||
| | ActivationPatchingConfigData; | ||
| | ActivationPatchingConfigData | ||
| | LogitLensIntroConfigData; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add cm-intro to ConfigData to keep chart/config unions aligned.
ChartType/ToolType now include "cm-intro", but ConfigData doesn’t represent its payload shape. That creates a typed hole when handling config data for this chart type.
Proposed fix
export type ConfigData =
| LensConfigData
| Lens2ConfigData
| PatchingConfig
| ActivationPatchingConfigData
- | LogitLensIntroConfigData;
+ | LogitLensIntroConfigData
+ | Record<string, never>; // cm-introAs per coding guidelines: "workbench/_web/src/types/**/*.ts: Frontend types must re-export nnsightful types as workbench-native names (e.g., LogitLensData as Lens2Data)."
🧰 Tools
🪛 GitHub Actions: Run Checks / Run Prettier
[warning] Prettier reported a formatting warning.
🤖 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 `@workbench/_web/src/types/charts.ts` around lines 70 - 75, Chart config union
ConfigData is missing the cm-intro payload type, causing a typed hole; add a
dedicated config type for the cm-intro chart (e.g., CmIntroConfigData or a
re-exported nnsightful type following the project naming convention) and include
it in the ConfigData union alongside LensConfigData, Lens2ConfigData,
PatchingConfig, ActivationPatchingConfigData, and LogitLensIntroConfigData;
ensure the new type matches the actual cm-intro payload shape and is exported
with the workbench-native name so ChartType/ToolType handling is fully typed.
jon-bell
added
the
preview-deploy
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
workbench/_web/src/components/transformer/UnembedComponent.tsx (1)
95-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit callback typings for D3 handlers to clear implicit-
anyerrors.Lines 95 and 180 fail strict TypeScript checks: the
eventparameter in the click handler and thethiscontext in the.each()callback are implicitlyany.Proposed fix
- .on("click", (event) => { + .on("click", (event: MouseEvent) => { event.stopPropagation(); setClickedComponent({ tokenIndex, layerIndex: numLayers - 1, componentType: "unembed", }); }); ... - g.selectAll("[data-component-id]").each(function () { + g.selectAll("[data-component-id]").each(function (this: d3.BaseType) { const element = d3.select(this);🤖 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 `@workbench/_web/src/components/transformer/UnembedComponent.tsx` at line 95, Add explicit TypeScript types to the D3 callbacks to remove implicit-any errors: change the click handler signature from .on("click", (event) => { ... }) to .on("click", (event: PointerEvent, d: unknown) => { ... }) (or PointerEvent | MouseEvent if you prefer) and change the .each(...) arrow callback to a function declaration that types this and its args, e.g. .each(function(this: HTMLElement /* or SVGElement */, datum: unknown, index: number) { ... }), so the handler and the per-element callback both have explicit typings and satisfy strict checks in UnembedComponent.tsx.workbench/_web/src/components/charts/ChartDisplay.tsx (1)
68-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix schema type annotation instead of adding runtime guards.
The root issue is that
workbench/_web/src/db/schema.sqlite.tsline 49 is missing the.$type<ConfigData>()annotation thatworkbench/_web/src/db/schema.pg.tsline 44 has. This causesConfig.datato be inferred asunknownin SQLite, leading to the TS2339 error on lines 68 and 75.Rather than adding type guards in ChartDisplay.tsx, fix the schema:
Recommended fix (in schema.sqlite.ts)
+ import type { ConfigData } from "`@/types/charts`"; ... export const configs = sqliteTable("configs", { id: text("id").primaryKey().$defaultFn(generateUUID), workspaceId: text("workspace_id").notNull(), - data: text("data", { mode: "json" }).notNull(), + data: text("data", { mode: "json" }).$type<ConfigData>().notNull(), type: text("type").notNull(), createdAt: integer("created_at", { mode: "timestamp" }) .$defaultFn(() => new Date()) .notNull(), });This aligns both database schemas and eliminates the type error at the source, making the code cleaner than runtime guards.
🤖 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 `@workbench/_web/src/components/charts/ChartDisplay.tsx` around lines 68 - 75, Config.data is being inferred as unknown because the SQLite schema is missing the type annotation; open schema.sqlite.ts and add the same .$type<ConfigData>() generic annotation used in schema.pg.ts to the Config table/column definition so Config.data is typed as ConfigData (i.e., append .$type<ConfigData>() to the schema builder for the Config.data field), which will remove the TS2339 errors in ChartDisplay.tsx for statisticType/metricType without adding runtime guards.workbench/_web/src/lib/api/chartApi.ts (1)
374-375:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReplace
nullwith empty arrays to matchActivationPatchingConfigDatatype declarations.
srcPosandtgtPosare declared asSourcePosition[]andnumber[]respectively, but initialized withnull, causing TypeScript compilation errors.Proposed fix
const defaultConfig: ActivationPatchingConfigData = { model: "", srcPrompt: "", tgtPrompt: "", - srcPos: null, - tgtPos: null, + srcPos: [], + tgtPos: [], };🤖 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 `@workbench/_web/src/lib/api/chartApi.ts` around lines 374 - 375, The object initialization sets srcPos and tgtPos to null which conflicts with their declared types (SourcePosition[] and number[]) in ActivationPatchingConfigData; update the initialization of srcPos and tgtPos to empty arrays ([]) instead of null so the values match the declared array types in the code that constructs/returns the ActivationPatchingConfigData object (look for the object containing srcPos and tgtPos in chartApi.ts).
♻️ Duplicate comments (1)
workbench/_web/src/lib/api/chartApi.ts (1)
301-303:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPass
workspaceIdinto sidebar invalidation in both new create hooks.These handlers currently call
queryKeys.charts.sidebar()with no args, which is a TS compile blocker and can miss workspace-scoped cache refresh.Proposed fix
export const useCreateLogitLensIntroChartPair = () => { @@ - onSuccess: () => { - queryClient.invalidateQueries({ queryKey: queryKeys.charts.sidebar() }); + onSuccess: (_, { workspaceId }) => { + queryClient.invalidateQueries({ + queryKey: queryKeys.charts.sidebar(workspaceId), + }); }, @@ export const useCreateCMIntroChartPair = () => { @@ - onSuccess: () => { - queryClient.invalidateQueries({ queryKey: queryKeys.charts.sidebar() }); + onSuccess: (_, { workspaceId }) => { + queryClient.invalidateQueries({ + queryKey: queryKeys.charts.sidebar(workspaceId), + }); },Also applies to: 314-316
🤖 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 `@workbench/_web/src/lib/api/chartApi.ts` around lines 301 - 303, The onSuccess handlers for the new create hooks call queryClient.invalidateQueries({ queryKey: queryKeys.charts.sidebar() }) without a workspace id; update both handlers to pass the current workspaceId into the cache key (e.g., queryKeys.charts.sidebar(workspaceId)) so the invalidation targets the workspace-scoped cache; locate the onSuccess blocks that call queryClient.invalidateQueries and replace queryKeys.charts.sidebar() with queryKeys.charts.sidebar(workspaceId) (ensure workspaceId is available in the hook scope or passed into the handler).
🧹 Nitpick comments (2)
workbench/_web/src/app/workbench/[workspaceId]/activation-patching/[chartId]/components/TokenSelector.tsx (1)
206-207: ⚡ Quick winReplace arbitrary
text-[10px]with Tailwind scale token.Use a standard size token (e.g.,
text-xs) for this badge class.As per coding guidelines, "Never use arbitrary
text-[Npx]values; use Tailwind's standard scale (text-xs,text-sm, etc.)".🤖 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 `@workbench/_web/src/app/workbench/`[workspaceId]/activation-patching/[chartId]/components/TokenSelector.tsx around lines 206 - 207, In the TokenSelector component replace the arbitrary Tailwind class text-[10px] on the badge span with a standard Tailwind size token (for example text-xs) so the badge uses the design system scale; update the span that renders {badge} (the element with className including text-[10px]) to use text-xs instead.workbench/_web/src/lib/queries/documentQueries.ts (1)
41-45: ⚡ Quick winUse
interfaceforLexicalNodeshape to match project TS conventions.This shape declaration should be an
interfaceper repo standards.As per coding guidelines, "Prefer interface for defining object shapes in TypeScript code".
🤖 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 `@workbench/_web/src/lib/queries/documentQueries.ts` around lines 41 - 45, Replace the type alias declaration "type LexicalNode = { type?: string; text?: unknown; children?: LexicalNode[] }" with an equivalent interface declaration "interface LexicalNode { type?: string; text?: unknown; children?: LexicalNode[] }" in documentQueries.ts so the shape follows the repo's TypeScript convention; keep the same optional properties and recursive children type exactly as originally declared.
🤖 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 `@workbench/_web/package.json`:
- Around line 46-50: The project is missing the d3 package in
workbench/_web/package.json causing imports in UnembedComponent.tsx,
EmbedComponent.tsx, and InteractiveTransformer.tsx (imported at the top of those
files) to fail; add d3 to the dependencies in workbench/_web/package.json (or
run npm/yarn to install and save it) so those modules resolve correctly and
update lockfile, ensuring the version you add is compatible with the existing
d3-* packages (e.g., d3-delaunay) used in the repo.
---
Outside diff comments:
In `@workbench/_web/src/components/charts/ChartDisplay.tsx`:
- Around line 68-75: Config.data is being inferred as unknown because the SQLite
schema is missing the type annotation; open schema.sqlite.ts and add the same
.$type<ConfigData>() generic annotation used in schema.pg.ts to the Config
table/column definition so Config.data is typed as ConfigData (i.e., append
.$type<ConfigData>() to the schema builder for the Config.data field), which
will remove the TS2339 errors in ChartDisplay.tsx for statisticType/metricType
without adding runtime guards.
In `@workbench/_web/src/components/transformer/UnembedComponent.tsx`:
- Line 95: Add explicit TypeScript types to the D3 callbacks to remove
implicit-any errors: change the click handler signature from .on("click",
(event) => { ... }) to .on("click", (event: PointerEvent, d: unknown) => { ...
}) (or PointerEvent | MouseEvent if you prefer) and change the .each(...) arrow
callback to a function declaration that types this and its args, e.g.
.each(function(this: HTMLElement /* or SVGElement */, datum: unknown, index:
number) { ... }), so the handler and the per-element callback both have explicit
typings and satisfy strict checks in UnembedComponent.tsx.
In `@workbench/_web/src/lib/api/chartApi.ts`:
- Around line 374-375: The object initialization sets srcPos and tgtPos to null
which conflicts with their declared types (SourcePosition[] and number[]) in
ActivationPatchingConfigData; update the initialization of srcPos and tgtPos to
empty arrays ([]) instead of null so the values match the declared array types
in the code that constructs/returns the ActivationPatchingConfigData object
(look for the object containing srcPos and tgtPos in chartApi.ts).
---
Duplicate comments:
In `@workbench/_web/src/lib/api/chartApi.ts`:
- Around line 301-303: The onSuccess handlers for the new create hooks call
queryClient.invalidateQueries({ queryKey: queryKeys.charts.sidebar() }) without
a workspace id; update both handlers to pass the current workspaceId into the
cache key (e.g., queryKeys.charts.sidebar(workspaceId)) so the invalidation
targets the workspace-scoped cache; locate the onSuccess blocks that call
queryClient.invalidateQueries and replace queryKeys.charts.sidebar() with
queryKeys.charts.sidebar(workspaceId) (ensure workspaceId is available in the
hook scope or passed into the handler).
---
Nitpick comments:
In
`@workbench/_web/src/app/workbench/`[workspaceId]/activation-patching/[chartId]/components/TokenSelector.tsx:
- Around line 206-207: In the TokenSelector component replace the arbitrary
Tailwind class text-[10px] on the badge span with a standard Tailwind size token
(for example text-xs) so the badge uses the design system scale; update the span
that renders {badge} (the element with className including text-[10px]) to use
text-xs instead.
In `@workbench/_web/src/lib/queries/documentQueries.ts`:
- Around line 41-45: Replace the type alias declaration "type LexicalNode = {
type?: string; text?: unknown; children?: LexicalNode[] }" with an equivalent
interface declaration "interface LexicalNode { type?: string; text?: unknown;
children?: LexicalNode[] }" in documentQueries.ts so the shape follows the
repo's TypeScript convention; keep the same optional properties and recursive
children type exactly as originally declared.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e4d9f272-1e86-4834-b120-969d5243a4d8
📒 Files selected for processing (27)
workbench/_web/instrumentation.tsworkbench/_web/package.jsonworkbench/_web/src/app/dev/logit-lens-intro/page.tsxworkbench/_web/src/app/workbench/[workspaceId]/activation-patching/[chartId]/components/TokenSelector.tsxworkbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/components/CMIntroArea.tsxworkbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/components/CMIntroDisplay.tsxworkbench/_web/src/app/workbench/[workspaceId]/components/ChartCard.tsxworkbench/_web/src/app/workbench/[workspaceId]/components/ChartCardsSidebar.tsxworkbench/_web/src/app/workbench/[workspaceId]/logit-lens-intro/[chartId]/components/LogitLensIntroArea.tsxworkbench/_web/src/app/workbench/[workspaceId]/logit-lens-intro/[chartId]/components/LogitLensIntroControls.tsxworkbench/_web/src/app/workbench/[workspaceId]/logit-lens-intro/[chartId]/components/LogitLensIntroDisplay.tsxworkbench/_web/src/app/workbench/[workspaceId]/logit-lens-intro/[chartId]/page.tsxworkbench/_web/src/app/workbench/page.tsxworkbench/_web/src/components/charts/ChartDisplay.tsxworkbench/_web/src/components/charts/heatmap/Heatmap.tsxworkbench/_web/src/components/transformer/EmbedComponent.tsxworkbench/_web/src/components/transformer/InteractiveTransformer.tsxworkbench/_web/src/components/transformer/UnembedComponent.tsxworkbench/_web/src/db/__tests__/local-db.test.tsworkbench/_web/src/lib/api/chartApi.tsworkbench/_web/src/lib/api/cmIntroApi.tsworkbench/_web/src/lib/api/logitLensIntroApi.tsworkbench/_web/src/lib/queries/documentQueries.tsworkbench/_web/src/lib/supabase/server.tsworkbench/_web/src/lib/utils.tsworkbench/_web/src/types/charts.tsworkbench/_web/tests/k6/utils.ts
✅ Files skipped from review due to trivial changes (4)
- workbench/_web/src/components/transformer/InteractiveTransformer.tsx
- workbench/_web/src/components/transformer/EmbedComponent.tsx
- workbench/_web/src/app/workbench/page.tsx
- workbench/_web/src/lib/supabase/server.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- workbench/_web/src/app/workbench/[workspaceId]/logit-lens-intro/[chartId]/page.tsx
- workbench/_web/src/app/workbench/[workspaceId]/components/ChartCard.tsx
- workbench/_web/src/types/charts.ts
- workbench/_web/src/app/workbench/[workspaceId]/logit-lens-intro/[chartId]/components/LogitLensIntroArea.tsx
- workbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/components/CMIntroDisplay.tsx
- workbench/_web/src/app/workbench/[workspaceId]/components/ChartCardsSidebar.tsx
- workbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/components/CMIntroArea.tsx
- workbench/_web/src/app/dev/logit-lens-intro/page.tsx
- workbench/_web/src/app/workbench/[workspaceId]/logit-lens-intro/[chartId]/components/LogitLensIntroDisplay.tsx
- workbench/_web/src/lib/api/logitLensIntroApi.ts
- workbench/_web/src/lib/api/cmIntroApi.ts
- workbench/_web/src/app/workbench/[workspaceId]/logit-lens-intro/[chartId]/components/LogitLensIntroControls.tsx
Comment on lines
46
to
50
| "d3-delaunay": "^6.0.4", | ||
| "dotenv": "^17.2.1", | ||
| "drizzle-orm": "^0.44.4", | ||
| "edulogitlens": "github:skysneakers/edulogitlens#8ccb422403a394a094b969a3fb2ce05fb0095a7e", | ||
| "framer-motion": "^12.23.22", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find UnembedComponent.tsx
find . -name "UnembedComponent.tsx" -o -name "UnembedComponent.ts" -o -name "UnembedComponent.jsx" 2>/dev/nullRepository: ndif-team/workbench
Length of output: 128
🏁 Script executed:
# Check the current package.json for d3 dependencies
if [ -f "workbench/_web/package.json" ]; then
grep -E '"d3' "workbench/_web/package.json"
fiRepository: ndif-team/workbench
Length of output: 96
🏁 Script executed:
# Search for d3 imports in the codebase
rg "import.*from ['\"]d3['\"]" --type ts --type tsx --type js --type jsxRepository: ndif-team/workbench
Length of output: 91
🏁 Script executed:
# Read UnembedComponent.tsx to check for d3 imports
cat -n "./workbench/_web/src/components/transformer/UnembedComponent.tsx" | head -50Repository: ndif-team/workbench
Length of output: 1627
🏁 Script executed:
# Retry rg search for d3 imports with correct file type
rg "import.*from ['\"]d3['\"]" --type ts --type jsRepository: ndif-team/workbench
Length of output: 334
🏁 Script executed:
# Also check for d3 in all imports more broadly
rg "from ['\"]d3" workbench/_web/src/ 2>/dev/null | head -20Repository: ndif-team/workbench
Length of output: 446
🏁 Script executed:
# Get full package.json dependencies section to see all d3-related packages
if [ -f "workbench/_web/package.json" ]; then
cat "workbench/_web/package.json" | grep -A 1 -B 1 d3
fiRepository: ndif-team/workbench
Length of output: 151
🏁 Script executed:
# Check if there's a package-lock.json or yarn.lock that might indicate recommended versions
if [ -f "workbench/_web/package-lock.json" ]; then
rg '"d3"' "workbench/_web/package-lock.json" -A 3 | head -20
elif [ -f "workbench/_web/yarn.lock" ]; then
rg '"d3"' "workbench/_web/yarn.lock" -A 3 | head -20
fiRepository: ndif-team/workbench
Length of output: 45
Add the d3 package to dependencies to fix module resolution.
Multiple components import d3 directly (UnembedComponent.tsx, EmbedComponent.tsx, InteractiveTransformer.tsx at line 4), but the package is missing from package.json. This causes a build failure.
Proposed fix
"d3-delaunay": "^6.0.4",
+ "d3": "^7.9.0",
"dotenv": "^17.2.1",📝 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.
Suggested change
| "d3-delaunay": "^6.0.4", | |
| "dotenv": "^17.2.1", | |
| "drizzle-orm": "^0.44.4", | |
| "edulogitlens": "github:skysneakers/edulogitlens#8ccb422403a394a094b969a3fb2ce05fb0095a7e", | |
| "framer-motion": "^12.23.22", | |
| "d3-delaunay": "^6.0.4", | |
| "d3": "^7.9.0", | |
| "dotenv": "^17.2.1", | |
| "drizzle-orm": "^0.44.4", | |
| "edulogitlens": "github:skysneakers/edulogitlens#8ccb422403a394a094b969a3fb2ce05fb0095a7e", | |
| "framer-motion": "^12.23.22", |
🤖 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 `@workbench/_web/package.json` around lines 46 - 50, The project is missing the
d3 package in workbench/_web/package.json causing imports in
UnembedComponent.tsx, EmbedComponent.tsx, and InteractiveTransformer.tsx
(imported at the top of those files) to fail; add d3 to the dependencies in
workbench/_web/package.json (or run npm/yarn to install and save it) so those
modules resolve correctly and update lockfile, ensuring the version you add is
compatible with the existing d3-* packages (e.g., d3-delaunay) used in the repo.
Contributor
|
🚀 Preview deployed
|
CMIntroArea now defaults the workspace model to Llama-3.1-8B (32 layers) once models load, instead of leaving it at index 0 (the 70B, 80 layers) — that was the source of the ~79-layer heatmap. Also added a comment in CMIntroDisplay explaining why the BOS token is deliberately NOT stripped here: CM interventions send absolute token positions to the backend (causal_mediation.py indexes the BOS-inclusive tokenization), so dropping position 0 in the display would patch the wrong token. BOS-hiding for CM must be done in the widget while keeping absolute positions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- cmIntroApi: skip target lens when targetPrompt is blank; CMIntroLensResult.target becomes nullable. - CMIntroArea: Run only requires source. Empty target -> single-heatmap view (widget handles rendering). - Bump edulogitlens to 2380b5e (single-prompt mode in CausalMediationExplorer). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CMIntroDisplay no longer hands stale or missing data to the CausalMediationExplorer (which silently falls back to mock data). It now compares the current source/target prompts against the snapshots the live data was computed for (lastRunSrcPrompt/lastRunTgtPrompt from the ephemeral lensResult, or lastRunSourcePrompt/lastRunTargetPrompt from the persisted chart row). On mismatch, or when no data exists yet, it renders a small skeleton-grid placeholder with a message telling the user to run / re-run the analysis. Also relaxes persistedData to treat target as optional (single-prompt mode persists with no target field). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The custom ContentComponent stripped reactour's default step navigation, so once a tour opened on the welcome step there was no way to advance. Adds a small footer with step counter, Prev/Next or Done, and a close button. Step triggers (textInput/click) still auto-advance when wired, but manual nav works for all steps now. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds tutorials/cmIntro.ts (8-step walkthrough of source/target prompts, how to drag for an intervention, how to read the result), the matching useCMIntroTutorial completion store, the auto-start hook + Tutorial button in CMIntroArea, and #cm-intro-* selectors on the controls and display. Mirrors the lens-intro tutorial pattern but scoped to CM only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A clean fork branch off skysneakers/edulogitlens main carrying only the CM-relevant widget commits: localhost-beacon removal, CausalMediationExplorer auto-fit + sync scroll, CM heatmap chevrons (in gutters), draggable Selected Position panel, crosshair + cone highlight as background tints behind the cells, single-prompt mode, About-this-view legend, and the isolation stacking fix. Does NOT include the LogitLensGrid changes (compact chevrons, autofit, onCellClick/highlight/onScroll/tooltipFor) — those were for lens-intro and are out of scope for this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Auto-focus the source-prompt textarea once on mount when the prompt is
empty (skipped when revisiting a chart with persisted prompts).
Adds a one-line helper under each prompt explaining its role:
* Source = the prompt to steal state from
* Target = the prompt to patch into (or leave blank for a single-prompt
lens view)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eatmaps * Clear button (X) appears next to source/target prompt when non-empty; resets both the prompt string and the cached tokens, focuses the textarea. * Blur with an empty prompt now clears the cached tokens too — previously the stale tokenized display would reappear after the user deleted text. * Bump edulogitlens to 631234c which raises HeatmapGrid max-height from 60vh to 82vh, so the existing autofit step calculation picks more rows and large heatmaps fit without scrolling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/page.tsx (1)
150-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass
lastRunSrcPrompt/lastRunTgtPromptin the desktopCMIntroDisplaypath too.Mobile passes these snapshots, desktop does not. That makes stale-heatmap detection inconsistent and can show outdated data after prompt edits on desktop.
🤖 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 `@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/page.tsx around lines 150 - 154, The desktop rendering of CMIntroDisplay is missing the lastRunSrcPrompt and lastRunTgtPrompt props, causing inconsistent stale-heatmap detection; update the CMIntroDisplay invocation in page.tsx to pass lastRunSrcPrompt and lastRunTgtPrompt (the same snapshots used on the mobile path) alongside sourcePrompt and targetPrompt so CMIntroDisplay receives both snapshot props for desktop as well as mobile.
♻️ Duplicate comments (4)
workbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/components/CMIntroArea.tsx (1)
270-273:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude target tokenization model in stale-model warning logic.
configModelUnavailablecurrently only checks the source side, so stale target tokenization won’t surface a warning.🤖 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 `@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/components/CMIntroArea.tsx around lines 270 - 273, The stale-model detection currently only compares srcTokenizedModel to selectedModel; update the logic in configModelUnavailable to also check targetTokenizedModel against selectedModel and surface whichever side is mismatched (srcTokenizedModel or targetTokenizedModel) instead of only srcTokenizedModel — i.e., if either srcTokenizedModel !== selectedModel or targetTokenizedModel !== selectedModel, set configModelUnavailable to the mismatched tokenization model (prefer returning the specific unavailable model name) so stale target tokenization triggers the warning too.workbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/components/CMIntroDisplay.tsx (1)
184-187:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t keep
resultDatacontrolled from persisted state after live runs begin.When
persistedData.resultexists,resultDatacan keep overriding in-session intervention output. Gate it to initial hydration context (or clear it once live lens/intervention flow is active).🤖 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 `@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/components/CMIntroDisplay.tsx around lines 184 - 187, persistedResultData (computed from persistedData.result via transformToEduFormat) is currently recomputed and can overwrite in-session resultData after live runs start; change it so persistedResultData only provides initial hydration: compute it once (e.g., store in a ref or state initializer inside CMIntroDisplay) or stop using persistedResultData when a live-run flag (e.g., liveLensActive / interventionActive) becomes true, and ensure any code that reads persistedResultData (or writes resultData) checks that flag so persistedData.result is not applied after live flow starts.workbench/_web/src/lib/api/cmIntroApi.ts (1)
63-167: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExport standalone async executors for CM Intro operations, then call them from
mutationFn.Both flows are still hook-only, which blocks non-hook callers from reusing core API logic without toast/hook wiring.
As per coding guidelines, "Export the bare async function alongside the mutation hook so features can call it without automatic toast error handling".
🤖 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 `@workbench/_web/src/lib/api/cmIntroApi.ts` around lines 63 - 167, The hooks currently embed the async logic (headers creation, runLogitLens/startAndPoll, setChartData) which prevents reuse; extract two exported standalone async executors (e.g., export async function executeCMIntroLogitLens(request: CMIntroLensRequest): Promise<CMIntroLensResult> and export async function executeCMIntroIntervention(request: CMIntroInterventionRequest): Promise<LogitLensIntroData>) that perform the same steps currently in mutationFn (use createUserHeadersAction, runLogitLens, startAndPoll, merge/getChartById, setChartData, etc.), then simplify useCMIntroLogitLens and useCMIntroIntervention mutationFn to call those executors and keep only hook-specific behavior (toasts, invalidateQueries). Reference the existing functions runLogitLens, startAndPoll, setChartData, getChartById and the mutation hooks useCMIntroLogitLens/useCMIntroIntervention when moving the logic.workbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/page.tsx (1)
73-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard debounced autosave against out-of-order async completion.
Overlapping
getChartById/setChartDatacycles can complete out of order and persist stale prompts. Add a monotonic request-sequence guard so only the latest scheduled save writes.🤖 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 `@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/page.tsx around lines 73 - 95, The debounced autosave in the useEffect (uses hydratedRef, chartId, getChartById, setChartData, PROMPT_AUTOSAVE_DEBOUNCE_MS) can have overlapping async saves finish out of order and overwrite newer prompts; add a monotonic sequence guard: create a ref (e.g., saveSeqRef) that you increment each time the timeout handler is scheduled, capture the current sequence number in the async handler, and before applying any writes (both after fetching existing via getChartById and immediately before calling setChartData) verify the captured sequence matches the latest saveSeqRef.current; if it doesn’t match, abort so only the latest scheduled save persists and queryClient.invalidateQueries is only called for the winning sequence.
🧹 Nitpick comments (3)
workbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/components/CMIntroDisplay.tsx (1)
18-18: ⚡ Quick winReplace
rounded-lgwith the feature’s standard radius token.Use
rounded-mdorroundedto stay consistent with the route’s radius system.As per coding guidelines, "Use
rounded-mdorroundedfor border radius; avoid mixingrounded-xl/rounded-lgwith the standard tokens".🤖 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 `@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/components/CMIntroDisplay.tsx at line 18, The div in CMIntroDisplay.tsx uses the non-standard class "rounded-lg"; update the border-radius token to the route's standard by replacing "rounded-lg" with "rounded-md" (or "rounded" if preferred) in the element that renders the skeleton container (the div with className "rounded-lg border bg-secondary/30 p-4 animate-pulse") so the component follows the feature's radius tokens.workbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/components/CMIntroArea.tsx (1)
362-371: ⚡ Quick winUse the shared
Buttonprimitive for Clear actions instead of raw<button>.These controls are custom-styled buttons in a feature area where the existing UI primitive should be used for consistency and accessibility defaults.
As per coding guidelines, "Always reach for existing
components/uiprimitives first before creating custom components (badge, button, card, checkbox, dialog, double-slider, dropdown-menu, input, label, mode-toggle, popover, resizable, scroll-area, select, separator, slider, sonner, switch, textarea, tooltip)".Also applies to: 401-410
🤖 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 `@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/components/CMIntroArea.tsx around lines 362 - 371, Replace the raw <button> used for the "Clear" action with the shared Button primitive to ensure consistent styling and accessibility: locate the Clear control in CMIntroArea (the button with onClick={handleClearSrc}, disabled={isRunning}, and child <X /> + "Clear") and swap it to use the Button component from components/ui (or the project’s Button export), keeping the same props/behavior (onClick -> handleClearSrc, disabled -> isRunning, title -> "Clear source prompt") and preserve the icon (<X />) and text content; apply the equivalent className/variant props on Button so visual styling and positioning remain unchanged. Ensure you do the same replacement for the similar control at the other occurrence referenced (lines ~401-410).workbench/_web/src/components/providers/TourProvider.tsx (1)
40-74: ⚡ Quick winUse
components/ui/Buttonfor tour action controls.
Prev,Next,Done, and close are custom buttons right now; switching to the shared button primitive keeps interaction, theme, and focus behavior consistent.As per coding guidelines, "Always reach for existing
components/uiprimitives first before creating custom components (badge, button, card, checkbox, dialog, double-slider, dropdown-menu, input, label, mode-toggle, popover, resizable, scroll-area, select, separator, slider, sonner, switch, textarea, tooltip)".🤖 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 `@workbench/_web/src/components/providers/TourProvider.tsx` around lines 40 - 74, Replace the raw <button> elements used for the tour controls with the shared Button primitive from components/ui/Button: import Button and swap the Prev/Next/Done/close buttons so they call the same handlers (onClick={() => setCurrentStep(currentStep - 1)}, onClick={() => setCurrentStep(currentStep + 1)}, onClick={() => setIsOpen(false)}) and preserve accessibility props (type, aria-label, title) and styling intent (use Button variants/size or pass className if needed); keep conditions using isFirst/isLast and the same children text ("Prev", "Next", "Done", "×") so focus, theming and interaction behavior come from the Button component rather than raw <button> elements.
🤖 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.
Outside diff comments:
In `@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/page.tsx:
- Around line 150-154: The desktop rendering of CMIntroDisplay is missing the
lastRunSrcPrompt and lastRunTgtPrompt props, causing inconsistent stale-heatmap
detection; update the CMIntroDisplay invocation in page.tsx to pass
lastRunSrcPrompt and lastRunTgtPrompt (the same snapshots used on the mobile
path) alongside sourcePrompt and targetPrompt so CMIntroDisplay receives both
snapshot props for desktop as well as mobile.
---
Duplicate comments:
In
`@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/components/CMIntroArea.tsx:
- Around line 270-273: The stale-model detection currently only compares
srcTokenizedModel to selectedModel; update the logic in configModelUnavailable
to also check targetTokenizedModel against selectedModel and surface whichever
side is mismatched (srcTokenizedModel or targetTokenizedModel) instead of only
srcTokenizedModel — i.e., if either srcTokenizedModel !== selectedModel or
targetTokenizedModel !== selectedModel, set configModelUnavailable to the
mismatched tokenization model (prefer returning the specific unavailable model
name) so stale target tokenization triggers the warning too.
In
`@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/components/CMIntroDisplay.tsx:
- Around line 184-187: persistedResultData (computed from persistedData.result
via transformToEduFormat) is currently recomputed and can overwrite in-session
resultData after live runs start; change it so persistedResultData only provides
initial hydration: compute it once (e.g., store in a ref or state initializer
inside CMIntroDisplay) or stop using persistedResultData when a live-run flag
(e.g., liveLensActive / interventionActive) becomes true, and ensure any code
that reads persistedResultData (or writes resultData) checks that flag so
persistedData.result is not applied after live flow starts.
In `@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/page.tsx:
- Around line 73-95: The debounced autosave in the useEffect (uses hydratedRef,
chartId, getChartById, setChartData, PROMPT_AUTOSAVE_DEBOUNCE_MS) can have
overlapping async saves finish out of order and overwrite newer prompts; add a
monotonic sequence guard: create a ref (e.g., saveSeqRef) that you increment
each time the timeout handler is scheduled, capture the current sequence number
in the async handler, and before applying any writes (both after fetching
existing via getChartById and immediately before calling setChartData) verify
the captured sequence matches the latest saveSeqRef.current; if it doesn’t
match, abort so only the latest scheduled save persists and
queryClient.invalidateQueries is only called for the winning sequence.
In `@workbench/_web/src/lib/api/cmIntroApi.ts`:
- Around line 63-167: The hooks currently embed the async logic (headers
creation, runLogitLens/startAndPoll, setChartData) which prevents reuse; extract
two exported standalone async executors (e.g., export async function
executeCMIntroLogitLens(request: CMIntroLensRequest): Promise<CMIntroLensResult>
and export async function executeCMIntroIntervention(request:
CMIntroInterventionRequest): Promise<LogitLensIntroData>) that perform the same
steps currently in mutationFn (use createUserHeadersAction, runLogitLens,
startAndPoll, merge/getChartById, setChartData, etc.), then simplify
useCMIntroLogitLens and useCMIntroIntervention mutationFn to call those
executors and keep only hook-specific behavior (toasts, invalidateQueries).
Reference the existing functions runLogitLens, startAndPoll, setChartData,
getChartById and the mutation hooks useCMIntroLogitLens/useCMIntroIntervention
when moving the logic.
---
Nitpick comments:
In
`@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/components/CMIntroArea.tsx:
- Around line 362-371: Replace the raw <button> used for the "Clear" action with
the shared Button primitive to ensure consistent styling and accessibility:
locate the Clear control in CMIntroArea (the button with
onClick={handleClearSrc}, disabled={isRunning}, and child <X /> + "Clear") and
swap it to use the Button component from components/ui (or the project’s Button
export), keeping the same props/behavior (onClick -> handleClearSrc, disabled ->
isRunning, title -> "Clear source prompt") and preserve the icon (<X />) and
text content; apply the equivalent className/variant props on Button so visual
styling and positioning remain unchanged. Ensure you do the same replacement for
the similar control at the other occurrence referenced (lines ~401-410).
In
`@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/components/CMIntroDisplay.tsx:
- Line 18: The div in CMIntroDisplay.tsx uses the non-standard class
"rounded-lg"; update the border-radius token to the route's standard by
replacing "rounded-lg" with "rounded-md" (or "rounded" if preferred) in the
element that renders the skeleton container (the div with className "rounded-lg
border bg-secondary/30 p-4 animate-pulse") so the component follows the
feature's radius tokens.
In `@workbench/_web/src/components/providers/TourProvider.tsx`:
- Around line 40-74: Replace the raw <button> elements used for the tour
controls with the shared Button primitive from components/ui/Button: import
Button and swap the Prev/Next/Done/close buttons so they call the same handlers
(onClick={() => setCurrentStep(currentStep - 1)}, onClick={() =>
setCurrentStep(currentStep + 1)}, onClick={() => setIsOpen(false)}) and preserve
accessibility props (type, aria-label, title) and styling intent (use Button
variants/size or pass className if needed); keep conditions using isFirst/isLast
and the same children text ("Prev", "Next", "Done", "×") so focus, theming and
interaction behavior come from the Button component rather than raw <button>
elements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4867892f-5def-4e95-bb29-c12a03c51f9a
⛔ Files ignored due to path filters (1)
workbench/_web/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
workbench/_web/package.jsonworkbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/components/CMIntroArea.tsxworkbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/components/CMIntroDisplay.tsxworkbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/page.tsxworkbench/_web/src/components/providers/TourProvider.tsxworkbench/_web/src/lib/api/cmIntroApi.tsworkbench/_web/src/stores/useCMIntroTutorial.tsworkbench/_web/src/tutorials/cmIntro.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/components/CMIntroArea.tsx:
- Around line 393-395: The helper copy in CMIntroArea.tsx renders a stray space
before the period because the JSX split places a newline between the span and
the period; update the JSX so the period is adjacent to the preceding text
(e.g., move the period inside the same text node or include it inside the <span
className="font-medium">steal state from</span> string) to eliminate the extra
space and render "steal state from." without altering styling.
In
`@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/components/CMIntroDisplay.tsx:
- Around line 156-159: The branch selection for target prompt provenance is
incorrect: change the conditional that sets liveTgtRun to use lensResult?.target
as the indicator of a live target (instead of lensResult?.source) so that
liveTargetRaw and liveTgtRun use the same provenance; ensure liveTgtRun picks
lastRunTgtPrompt when lensResult?.target is present, otherwise fall back to
persistedData?.lastRunTargetPrompt.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f0ce0c2-c6c7-49f8-a5b7-62906d34f7d6
📒 Files selected for processing (3)
workbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/components/CMIntroArea.tsxworkbench/_web/src/app/workbench/[workspaceId]/cm-intro/[chartId]/components/CMIntroDisplay.tsxworkbench/_web/src/components/providers/TourProvider.tsx
Comment on lines
+393
to
+395
| The prompt you want to <span className="font-medium">steal state from</span> | ||
| . Pick something with a clear, specific prediction — its internal | ||
| activations will be the source of the patch. |
There was a problem hiding this comment.
Fix the stray space before the period in the helper copy.
This JSX split renders as steal state from . Pick... because the newline before . collapses to a space.
Suggested fix
- The prompt you want to <span className="font-medium">steal state from</span>
- . Pick something with a clear, specific prediction — its internal
+ The prompt you want to <span className="font-medium">steal state from</span>.
+ Pick something with a clear, specific prediction — its internal🤖 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
`@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/components/CMIntroArea.tsx
around lines 393 - 395, The helper copy in CMIntroArea.tsx renders a stray space
before the period because the JSX split places a newline between the span and
the period; update the JSX so the period is adjacent to the preceding text
(e.g., move the period inside the same text node or include it inside the <span
className="font-medium">steal state from</span> string) to eliminate the extra
space and render "steal state from." without altering styling.
Comment on lines
+156
to
+159
| const liveTgtRun = | ||
| lensResult?.source != null | ||
| ? (lastRunTgtPrompt ?? null) | ||
| : (persistedData?.lastRunTargetPrompt ?? null); |
There was a problem hiding this comment.
Use the target branch to choose target prompt provenance.
liveTargetRaw falls back on lensResult?.target, but liveTgtRun falls back on lensResult?.source. That can pair persisted target data with the current run's target prompt snapshot and let stale target output look fresh.
Suggested fix
const liveTgtRun =
- lensResult?.source != null
+ lensResult?.target != null
? (lastRunTgtPrompt ?? null)
: (persistedData?.lastRunTargetPrompt ?? null);🤖 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
`@workbench/_web/src/app/workbench/`[workspaceId]/cm-intro/[chartId]/components/CMIntroDisplay.tsx
around lines 156 - 159, The branch selection for target prompt provenance is
incorrect: change the conditional that sets liveTgtRun to use lensResult?.target
as the indicator of a live target (instead of lensResult?.source) so that
liveTargetRaw and liveTgtRun use the same provenance; ensure liveTgtRun picks
lastRunTgtPrompt when lensResult?.target is present, otherwise fall back to
persistedData?.lastRunTargetPrompt.
…left) Co-Authored-By: Claude Opus 4.7 (1M context) <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.
Logit lens & activation patching visualizations
Summary by CodeRabbit
New Features
Improvements
Chores