refactor(lana): use Salesforce Services - #951
Conversation
| const logFilePath = this.getLogFilePath(ws, logFileId); | ||
| const writeLogFile = this.writeLogFile(ws, logFilePath); | ||
| return LogView.createView(context, writeLogFile, logFilePath); | ||
| const logFilePath = join( |
There was a problem hiding this comment.
I know this was already hard coded this but does @salesforce/vscode-services expose the default path eg sf/ or sfdx/ etc?
It would be more robust if it changes in future.
| import * as ManagedRuntime from 'effect/ManagedRuntime'; | ||
| import { extensions } from 'vscode'; | ||
|
|
||
| import type { SalesforceVSCodeServicesApi } from '@salesforce/vscode-services'; |
There was a problem hiding this comment.
Seem like the types might be broken and needs fixing. The index.d.ts just has relative exports from files in the monorepo. I looked in the out dir onnpm
| return getRuntime().runPromise(ApexLogService.getLogBody(logId)); | ||
| } | ||
|
|
||
| export function readFile(uri: Uri | string): Promise<string> { |
There was a problem hiding this comment.
I haven't dug into it but could we use vscode.workspace.fs instead of readFile, writeFile and fileOrFolderExists from sf extension services?
There was a problem hiding this comment.
With a hard dependency on services, then deferring to FS service is doable. If you want to run w/o services as a hard dependency, then using workspace.fs is the right move.
| } | ||
|
|
||
| servicesApi = extension.isActive ? extension.exports : await extension.activate(); | ||
| runtime = ManagedRuntime.make( |
There was a problem hiding this comment.
Could the extension expose a pre-built runtime or promise-returning wrappers? If so we could drop the effect dependendcy and save 20% on the bundle size.
There was a problem hiding this comment.
Will need to discuss with team
| "module": "ESNext", | ||
| "noEmit": true, | ||
| "types": ["jest"], | ||
| "types": ["jest", "node"], |
| { | ||
| "compilerOptions": { | ||
| "lib": ["ES2022"], | ||
| "lib": ["ES2022", "DOM"], |
There was a problem hiding this comment.
I wonder if we remove node from types and add "WebWorker" to lib to keep node stuff out?
|
I will be studying the required changes in Salesforce Services separately and will follow up once I understand the upstream API changes and sequencing. |
…llows your row (#973) # 📝 PR Overview A merged call-tree row was identified by joining its bucket keys into a string, which was most of the cost of the grid builds. Separately, a bottom-up caller row reported the calls it counts rather than the frames it is, so every caller depth highlighted the same leaf frames however deep you picked. The grids now group on the interned key ids the inspector already uses, and a row now reports the frames it stands for, so stepping down the callers walks the highlight up the stack. Three navigation bugs found while walking that change are fixed here too. ## 🛠️ Changes made - **Grid builds on interned key ids** — both builders take the log's `KeyPathIds` and group on integers instead of joined key strings. On a 95MB log (864,216 lines, 431,307 calls) `toBottomUpTree` goes 497ms → 377ms and `toAggregatedCallTree` 337ms → 308ms. Medians of four runs; run-to-run variance on this log is about 50ms, so read the second figure as directional. - **A frame outside the log's own index is keyed but not cached** — `keyIdOf` wrote past the end of its `Int32Array`, which lands as an ordinary property, so every frame built rather than parsed read back the first one's id. - **The Analysis reveal reads no occurrence** — it found its bucket by scanning the 431,307 occurrences the root buckets hold between them, then listed every active row again to test one filter. One key compare over the top-level rows and one boolean read now do it. - **A bottom-up row highlights its own frames** — `frameEventIndexes` climbs the row's own path depth from each counted call and dedupes, so the flame chart and call tree point at the frames the row is. Details still describes the calls it counts, which ride on a field of their own. - **The keyboard survives a tree-control click** — the control is not focusable, so clicking it dropped focus, and the key bindings only answer while the table body holds it: the arrows scrolled the table instead of moving through it. Focus now returns on every pointer expand and collapse, not only the first one before any row is selected. - **A bucket descent waits for the render it needs** — the descent skipped the wait entirely for any row already open, so it read empty children and fell back to that row: picking a deep inspector row landed on one of its callers and needed a second click. It now waits for any row whose children have not arrived. - **Analysis moves to a picked inspector row** — it marked the bucket but never scrolled to it, because only a finding click revealed. It now reveals on a sticky locate and then marks, as the Call Tree does. ## 🧩 Type of change (check all applicable) - [x] 🐛 Bug fix - something not working as expected - [ ] ✨ New feature – adds new functionality - [ ] ♻️ Refactor - internal changes with no user impact - [x] ⚡ Performance Improvement - [ ] 📝 Documentation - README or documentation site changes - [ ] 🔧 Chore - dev tooling, CI, config - [ ] 💥 Breaking change ## 📷 Screenshots / gifs / video [optional] N/A. Nothing new appears on screen: what changes is build timing and which frames light up. ## 🔗 Related Issues None. ## ✅ Tests added? - [x] 👍 yes - [ ] 🙅 no, not needed - [ ] 🙋 no, I need help ## 📚 Docs updated? - [ ] 🔖 README.md - [x] 🔖 CHANGELOG.md - [ ] 📖 help site - [ ] 🧪 Marked any pre-release-only features - [ ] 🙅 not needed Two entries, for the keyboard and navigation fixes. The highlight and Analysis fixes correct the Inspector, which is unreleased, so they belong to its existing entry rather than a new one. ## Anything else we need to know? [optional] **Where to start.** `log-viewer/src/core/log/keyPathIds.ts` carries the vocabulary; read its class doc first. The two id spaces matter: the aggregated build composes outermost-first and the bottom-up build innermost-first, so ids from the two directions must never be compared. **Two questions, two accessors.** `locatableEventIndexes` is the calls a row counts, which its totals describe. `frameEventIndexes` is the frames the row is, which a highlight points at. They differ only for a bottom-up caller row. **Test plan.** - `pnpm lint` and `pnpm test`. - Call Tree, Bottom Up: expand a method to its callers. Totals and call counts read as before. Hover depths 2, 3 and 4: the lit frames walk up the stack a level at a time, and `Called by` names the row hovered. - Click a row's expand arrow, then press the up and down arrows: the selection moves and the table does not scroll on its own. - Aggregated: pick a deep row from the inspector's call tree. It lands on that row first time, not on one of its callers. - Analysis: pick a row in the inspector's call tree. The grid scrolls to the bucket and selects it. Picking a bucket the Show Details filter hides turns that filter off, as a finding click already did. - Both themes, and the Inspector docked at the side and at the bottom. **Known unrelated failure locally.** `lana/src/services/__tests__/servicesRuntime.test.ts` cannot resolve `effect` in a worktree that has not been installed since #951. No `lana` file is touched here. **Reverted during review.** A first attempt routed that wait through `RowNavigation`'s pending-render flag. Tabulator dispatches `renderStarted` and `renderComplete` in one synchronous call, so the flag always read false where the wait was awaited and the indirection bought nothing, while its one live branch could only be entered by a `renderStarted` whose `renderComplete` never came — which one throwing subscriber causes, since `_dispatch` has no try/catch and five modules subscribe. The wait would then never settle. The inline wait is back. **Follow-ups, not in this PR.** - The reverse direction still marks on the old rule: hovering a frame marks the rows whose leaf it is, rather than the rows whose own frame it is. Doing it properly needs a walk of that frame's subtree. - The reveal-then-mark handler is now shared in shape by four views but copied in two. `DatabaseView` and `ApexLogTimeline` mark without revealing, so "does a pick move this view" deserves to be an argument rather than a property of which handler was copied last.
…step (#974) # 📝 PR Overview Picking a row in the inspector's call tree moves the tab's grid to the bucket it names, and the move re-renders the rows the mark sits on, so the mark has to be applied after it. The Call Tree applied the mark it had read *before* the move. Dropping the pick while the move ran cleared the mark, and the move then put it back with nothing picked; hovering another row instead had that hover's own mark stripped and never re-applied, leaving the grid unmarked until the pointer moved again. The mark now reads what the inspector is pointing at when the move settles, which is the emphasis the view already keeps, so there is no second copy of that truth to fall out of step. The same handler was written four times, once per tab, with the defect in two of the copies; all four now share one. ## 🛠️ Changes made - **One handler for four tabs** — `inspectorLocateHandler` in `log-viewer/src/components/inspectorLocate.ts` replaces the hand-written `inspector:locate` handlers in `CalltreeView`, `AnalysisView`, `DatabaseView` and `ApexLogTimeline`. Whether a pick moves a view is now an argument rather than a property of which handler was copied last. - **The mark is read, not restored** — the deferred mark takes `InspectorEmphasis.current()` when the move settles, so a report that arrived during the move wins and a dropped pick stays dropped. `Escape` reaches a view as `selection:clear`, which never passes through this handler, so anything that tracked reports here rather than on the emphasis would have missed it. - **A failed move no longer leaks** — the move is awaited in a `try`, and the mark goes on either way: it says where the frames are whether the view reached them or not. Previously a rejection surfaced as an unhandled rejection. - **`EventDetail<K>`** — exported from `EventBus.ts` so a shared handler can be typed against one event's payload without lifting that payload out of `EventMap`, which stays where each event is described. ## 🧩 Type of change (check all applicable) - [x] 🐛 Bug fix - something not working as expected - [ ] ✨ New feature – adds new functionality - [x] ♻️ Refactor - internal changes with no user impact - [ ] ⚡ Performance Improvement - [ ] 📝 Documentation - README or documentation site changes - [ ] 🔧 Chore - dev tooling, CI, config - [ ] 💥 Breaking change ## 📷 Screenshots / gifs / video [optional] N/A. What changes is whether a mark is still on screen after a pick moves the grid. ## 🔗 Related Issues None. ## ✅ Tests added? - [x] 👍 yes - [ ] 🙅 no, not needed - [ ] 🙋 no, I need help ## 📚 Docs updated? - [ ] 🔖 README.md - [x] 🔖 CHANGELOG.md - [ ] 📖 help site - [ ] 🧪 Marked any pre-release-only features - [ ] 🙅 not needed No entry: the Inspector is unreleased, so this belongs to its existing entry rather than a new one. The box is ticked to record that it was considered. ## Anything else we need to know? [optional] **Where to start.** `log-viewer/src/components/inspectorLocate.ts` is the whole of it, 46 lines. `log-viewer/src/components/inspectorEmphasis.ts` is deliberately untouched: it stays pure state, and the new module composes it. **Test plan.** - \`pnpm lint\` and \`pnpm test\`. - Call Tree tab, inspector Call Tree → Bottom Up. Click a caller row, then press \`Escape\` before the scroll settles: the mark must stay gone. - Same again, but hover a different inspector row instead of pressing \`Escape\`: the hovered row's frames must be marked once the move settles, not the picked row's, and the grid must not be left unmarked. - Timeline: hover and pick inspector rows. The chart dims around them and must not pan. - Database: hover and pick. Statement rows mark, and the grid must not scroll. - Analysis: pick moves the grid and marks. Hover marks only. - \`Escape\` on each tab clears the selection and any held mark. **Known unrelated failure locally.** \`lana/src/services/__tests__/servicesRuntime.test.ts\` cannot resolve \`effect\` in a worktree not installed since #951. No \`lana\` file is touched here, and it passes in CI. **Follow-ups, not in this PR.** - **The grid mark is a one-shot DOM sweep.** \`LocatedRowMarker.mark\` adds a class to the rows present at that instant, and the row formatters stamp the row id but never re-apply the class, so scrolling a grid while a picked inspector row is lit loses the mark. Making the marker hold the wanted id set and having the formatter apply the class would fix that and delete the await-then-mark ordering this PR gets right by hand. - **The source filter is now in nine places.** Every \`DetailSource\`-carrying event is filtered the same way in each view; an \`eventBus.onSource\` would fold all nine, and with it the four near-identical \`inspector:reveal\` handlers, so one gesture stops needing two subscriptions per view. - **A stale move is not abandoned.** Clicking two inspector rows quickly lets the earlier move settle last and scroll away from the newer row. The mark is now correct either way, but the scroll is not; a Tabulator expand and scroll is not cancellable, so this needs its own approach. - **Whether the Database grids and the flame chart should move on a merged pick.** Both can. They do not today, and the shared handler makes that an explicit argument rather than an accident, but changing it is a product decision.
… rendered ones (#975) # 📝 PR Overview The mark that shows where an inspector row's frames sit was a one-shot DOM sweep over the rows a table had rendered. Tabulator builds a row's element on its first render, so the sweep could not reach a row that had never been on screen. Pick a row, then scroll down to a marked row below the viewport or expand a tree row whose children are built afterwards, and those rows arrive unmarked. The mark now belongs to the table rather than to a list of elements: the marker records the ids it wants, and the row formatter lights a row as it stamps it. That also removes the reason a pick's mark had to wait for the view to finish moving, so the handler #974 added loses its ordering, its awaits and three of its tests. ## 🛠️ Changes made - **The mark is declarative** — `LocatedRowMarker` records the wanted ids per table host, and `rowIndexStamper` / `stampRowPath` light a row as they stamp it. Every `rowFormatter` in the app already routes through one of those two, so no table factory needed a marker plumbed into it. - **The sweep un-lights as well as lights** — it toggles rather than adds. Tabulator re-uses a row's element rather than rebuilding it, so a row can come back carrying a mark that has since moved, and a view that switches tables no longer leaves the one it left marked. - **Mark before moving** — `inspectorLocateHandler` marks, then asks the view to move, and waits for nothing. A row the move renders lights itself. Gone with the ordering: the async body, the emphasis read-back, and the three tests that only covered which report won a race. - **A failed move is still answered**, so a view that cannot reach a frame leaves no unhandled rejection. ## 🧩 Type of change (check all applicable) - [x] 🐛 Bug fix - something not working as expected - [ ] ✨ New feature – adds new functionality - [x] ♻️ Refactor - internal changes with no user impact - [ ] ⚡ Performance Improvement - [ ] 📝 Documentation - README or documentation site changes - [ ] 🔧 Chore - dev tooling, CI, config - [ ] 💥 Breaking change ## 📷 Screenshots / gifs / video [optional] N/A. What changes is whether a row that was never on screen is marked when you reach it. ## 🔗 Related Issues Follows #974, which added the shared handler this simplifies. ## ✅ Tests added? - [x] 👍 yes - [ ] 🙅 no, not needed - [ ] 🙋 no, I need help Four new `LocatedRowMarker` cases: a row arriving after the mark lights itself, a re-used element loses a stale mark, a table nothing has marked is left alone, and a table the mark has left stops lighting rows. ## 📚 Docs updated? - [ ] 🔖 README.md - [x] 🔖 CHANGELOG.md - [ ] 📖 help site - [ ] 🧪 Marked any pre-release-only features - [ ] 🙅 not needed No entry: the Inspector is unreleased, so this belongs to its existing entry. Ticked to record that it was considered. ## Anything else we need to know? [optional] **Where to start.** `log-viewer/src/components/locatedRow.ts` — `wantedByHost`, `stamp` and `sweep` are the whole mechanism. `inspectorLocate.ts` is what falls out of it, now 16 lines with no async. **What does not happen, since it reads as though it should.** A class on a row element survives ordinary scrolling: `Row.create()` is guarded by `this.created`, `Row.initialize()` deletes cells but re-uses the element, `RowManager.styleRow` adds and removes parity classes rather than assigning `className`, and our renderer only detaches and re-attaches the element. The gap is the first render, not a later one. **Why a walk up the DOM.** `stamp` finds its table by walking parents until it meets a marked host. The alternative was threading the view's marker through four table factories to reach the formatters. The walk is a handful of nodes per row per render, against cell rendering that is orders of magnitude more, and it keeps the change inside one file. **Test plan.** - \`pnpm lint\` and \`pnpm test\`. - Call Tree tab, inspector Call Tree → Bottom Up. Pick a caller row, then scroll the grid down past what was on screen: rows the pick names are marked when you reach them. That is the fix. - Expand a tree row under a marked row: the children it builds arrive marked where they should be. - Pick a row and press \`Escape\` quickly: the mark stays gone, including on rows you scroll to afterwards. - Pick a row, then hover a different one: the hovered row's frames end up lit. - Timeline dims without panning; the Database grids mark without scrolling; Call Tree and Analysis move and mark. - Both themes, Inspector docked at the side and at the bottom. **Known unrelated failure locally.** \`lana/src/services/__tests__/servicesRuntime.test.ts\` cannot resolve \`effect\` in a worktree not installed since #951. No \`lana\` file is touched here, and it passes in CI. **Follow-ups, not in this PR.** - **The source filter is in nine places.** Every \`DetailSource\`-carrying event is filtered the same way in each view; an \`eventBus.onSource\` would fold all nine, and with it the four near-identical \`inspector:reveal\` handlers, so one gesture stops needing two subscriptions per view. - **A stale move is not abandoned.** Clicking two inspector rows quickly lets the earlier move settle last and scroll away from the newer row. The mark is right either way; the scroll is not, and a Tabulator expand and scroll is not cancellable. - **Whether the Database grids and the flame chart should move on a merged pick.** Both can. The shared handler makes that an explicit argument rather than an accident, but changing it is a product decision.
PR overview
Stack 1 of 4. Replaces direct Salesforce SDK integration with Salesforce Services while preserving the desktop extension.
Changes made
Type of change
Related issues
related W-23939830
Validation