Skip to content

refactor: centralize shared render constants#135

Open
benvinegar wants to merge 1 commit intomainfrom
pi/todo-416f785b-constants-types
Open

refactor: centralize shared render constants#135
benvinegar wants to merge 1 commit intomainfrom
pi/todo-416f785b-constants-types

Conversation

@benvinegar
Copy link
Copy Markdown
Member

Summary

  • add src/core/constants.ts for shared diff, layout, scroll, watch, and theme constants
  • replace repeated literals in App, DiffPane, loaders, Pierre rendering, and the vertical scrollbar with shared constants
  • add explicit Pierre helper return types where they were still inferred

Testing

  • bun run typecheck
  • bun test

This PR description was generated by Pi using OpenAI GPT-5

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 30, 2026

Greptile Summary

This PR is a clean refactoring that introduces src/core/constants.ts as the single source of truth for diff, layout, scroll, watch, and theme constants previously scattered as inline literals across five files. No logic is changed — every replacement is a direct 1-to-1 substitution with an identically-valued named constant.

Key changes:

  • DIFF_CONTEXT_LINES = 3 replaces two hardcoded 3 literals in loaders.ts, keeping parseDiffFromFile and createTwoFilesPatch in sync
  • UI_LAYOUT_CONSTANTS (6 values) replaces local const declarations inside AppShell, and introduces the previously unnamed SIDEBAR_DEFAULT_WIDTH = 34
  • UI_SCROLL_CONSTANTS (4 values) replaces inline numbers in DiffPane and VerticalScrollbar; the SCROLL_RESTORE_RETRY_DELAYS_MS array is now readonly [0, 16, 48] (due to as const) — ReadonlyArray.map() is fully supported so there is no type or runtime issue
  • THEME_CONSTANTS.RESERVED_PIERRE_TOKEN_COLORS replaces the identical as const object in pierre.ts; the key-lookup type inference in normalizeHighlightedColor is unaffected
  • Explicit return types were added to six previously-inferred helper functions in pierre.ts

Confidence Score: 5/5

Safe to merge — purely mechanical constant extraction with no behavior changes

Every replacement is a 1-to-1 substitution of an identical value; TypeScript types are preserved (including as const narrowing and ReadonlyArray.map() compatibility); no P0 or P1 findings

No files require special attention

Important Files Changed

Filename Overview
src/core/constants.ts New file centralizing diff, layout, scroll, watch, and theme constants — values are correct and groupings are logical
src/core/loaders.ts Replaces two hardcoded 3 literals with DIFF_CONTEXT_LINES; both call-sites (parseDiffFromFile and createTwoFilesPatch) remain consistent
src/ui/App.tsx Five local layout constants replaced with UI_LAYOUT_CONSTANTS destructuring; SIDEBAR_DEFAULT_WIDTH (34) and WATCH_POLL_INTERVAL_MS (250) added and values match originals
src/ui/components/panes/DiffPane.tsx Replaces three inline scroll constants with UI_SCROLL_CONSTANTS; SCROLL_RESTORE_RETRY_DELAYS_MS is readonly [0,16,48] from as const, and ReadonlyArray.map() is valid — no runtime or type issue
src/ui/components/scrollbar/VerticalScrollbar.tsx Module-level HIDE_DELAY_MS now sourced from UI_SCROLL_CONSTANTS.SCROLLBAR_HIDE_DELAY_MS; value (2000 ms) unchanged
src/ui/diff/pierre.ts Four magic literals replaced with named constants; Highlighter type alias and explicit return types added for normalizeHighlightedColor, flattenHighlightedLine, makeSplitCell, makeStackCell, prepareHighlighter, and queueHighlightedDiff

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    C[src/core/constants.ts]

    C -->|DIFF_CONTEXT_LINES| L[src/core/loaders.ts]
    C -->|UI_LAYOUT_CONSTANTS\nWATCH_POLL_INTERVAL_MS| A[src/ui/App.tsx]
    C -->|UI_SCROLL_CONSTANTS| D[src/ui/components/panes/DiffPane.tsx]
    C -->|UI_SCROLL_CONSTANTS| V[src/ui/components/scrollbar/VerticalScrollbar.tsx]
    C -->|DEFAULT_TAB_SIZE\nHIGHLIGHTER_PREFERRED\nTHEME_CONSTANTS\nTOKENIZE_MAX_LINE_LENGTH| P[src/ui/diff/pierre.ts]
Loading

Reviews (1): Last reviewed commit: "refactor: centralize shared render const..." | Re-trigger Greptile

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant