Skip to content

fix: address multi-pane review concerns#2443

Open
deadlyjack wants to merge 1 commit into
feat/multi-pane-viewfrom
fix/pr-2416-greptile-concerns
Open

fix: address multi-pane review concerns#2443
deadlyjack wants to merge 1 commit into
feat/multi-pane-viewfrom
fix/pr-2416-greptile-concerns

Conversation

@deadlyjack

Copy link
Copy Markdown
Member

No description provided.

@greptile-apps

greptile-apps Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR addresses two review concerns from the multi-pane feature branch: a constant rename for clarity in the drag-tab handler, and a behavioral fix to withPaneEditorContext so async callbacks keep the pane's editor context alive for their full duration.

  • editorFileTab.js: REORDER_DURATION and RELEASE_DURATION are renamed to include a _SECONDS suffix at their declaration and both call sites — no logic change.
  • editorManager.js: The previous try/finally in withPaneEditorContext was incorrect for async callbacks because JavaScript's finally runs synchronously when a Promise is returned, restoring editor/$container/touchSelectionController before the awaited work completed. The new code duck-types the return value and, for thenables, chains .finally(restoreContext) on the resolved promise so context is only unwound after the async callback settles.

Confidence Score: 4/5

Safe to merge; the async context fix is correct and an improvement over the previous try/finally approach, and the constant rename is purely cosmetic.

Both changes are straightforward and well-scoped. The withPaneEditorContext rewrite correctly defers restoreContext for async callbacks via .finally(), fixing a real flaw in the old implementation. No current caller passes an async callback, so behavior is unchanged in practice today. The latent concern around module-level globals remaining overridden during an async window is worth noting for future callers but does not affect the current code paths.

src/lib/editorManager.js — specifically withPaneEditorContext and any future async callers that may be added as the multi-pane feature evolves.

Important Files Changed

Filename Overview
src/handlers/editorFileTab.js Renames REORDER_DURATION and RELEASE_DURATION constants to include a _SECONDS suffix at their declaration and two usage sites — pure cosmetic clarity improvement, no logic change.
src/lib/editorManager.js Replaces try/finally in withPaneEditorContext with an explicit promise-aware pattern so async callbacks keep the pane context alive until the returned promise settles instead of having it restored synchronously on the same tick.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant withPaneEditorContext
    participant Globals as editor / $container globals
    participant Callback

    Caller->>withPaneEditorContext: call(pane, asyncCallback)
    withPaneEditorContext->>Globals: save previous context
    withPaneEditorContext->>Globals: set pane context
    withPaneEditorContext->>Callback: invoke callback()
    Callback-->>withPaneEditorContext: returns Promise (pending)
    Note over Globals: pane context still active
    withPaneEditorContext->>Caller: return Promise.resolve(result).finally(restoreContext)
    Note over Callback: async work continues with correct pane context
    Callback-->>withPaneEditorContext: Promise settles
    withPaneEditorContext->>Globals: restoreContext() — previous context restored
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Caller
    participant withPaneEditorContext
    participant Globals as editor / $container globals
    participant Callback

    Caller->>withPaneEditorContext: call(pane, asyncCallback)
    withPaneEditorContext->>Globals: save previous context
    withPaneEditorContext->>Globals: set pane context
    withPaneEditorContext->>Callback: invoke callback()
    Callback-->>withPaneEditorContext: returns Promise (pending)
    Note over Globals: pane context still active
    withPaneEditorContext->>Caller: return Promise.resolve(result).finally(restoreContext)
    Note over Callback: async work continues with correct pane context
    Callback-->>withPaneEditorContext: Promise settles
    withPaneEditorContext->>Globals: restoreContext() — previous context restored
Loading

Reviews (1): Last reviewed commit: "fix: address multi-pane review concerns" | Re-trigger Greptile

Comment thread src/lib/editorManager.js
Comment on lines +2694 to +2695
if (result && typeof result.then === "function") {
return Promise.resolve(result).finally(restoreContext);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Module-level globals remain overridden during async window

When the callback returns a Promise the module-level editor, $container, and touchSelectionController stay pointing at the pane's values until the promise settles. If any synchronous code (event handlers, scheduled timers, etc.) runs in that window and reads those globals, it will see the pane's context rather than the active editor's. This is not a regression from the old try/finally (which restored too early), but it is a latent race that could surface if more async callers are added. A comment here noting the interleaving risk would help future readers.

All three current call sites pass synchronous lambdas, so there is no present breakage — just something worth documenting.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

1 participant