Skip to content

paranext-core part of suppporting dev env support folder switching#2283

Open
katherinejensen00 wants to merge 2 commits into
mainfrom
Fix-manual-sync-in-dev
Open

paranext-core part of suppporting dev env support folder switching#2283
katherinejensen00 wants to merge 2 commits into
mainfrom
Fix-manual-sync-in-dev

Conversation

@katherinejensen00
Copy link
Copy Markdown
Contributor

@katherinejensen00 katherinejensen00 commented May 16, 2026

Code Review Summary

Branch: Fix-manual-sync-in-dev

Base: origin/main

Date: 2026-05-15

Review model: Claude Sonnet 4.6

Files changed: 3

Overview

This change is a focused refactor that extracts the hardcoded string 'platform.bible' (the user app-data folder name) into a named exported constant PRODUCT_FOLDER_NAME in src/node/utils/util.ts. The getAppDir function now derives its path from this constant rather than a duplicate string literal, and the corresponding test is updated to match. The lib/papi-dts/papi.d.ts type surface is updated to reflect the new export.

This is part of a larger effort to correctly identify and use the right application data path depending on whether Platform.Bible is running in a development or production environment. The change is a pure refactor — getAppDir produces the same path value as before and there is no behavioral change at runtime.

A secondary fix was included: a stale {{ productInfo.name }} template placeholder in the TSDoc for getAppDir was replaced with the actual resolved value .platform.bible.

API Changes

  • node/utils/util (in papi.d.ts): Added PRODUCT_FOLDER_NAME export (string constant 'platform.bible')
  • node/utils/util (in papi.d.ts): Updated TSDoc comment on getAppDir — replaced stale template variable {{ productInfo.name }} with resolved value .platform.bible

Findings

Critical — Must address before merge

None.

Important — Should address before merge

None.

Minor — Consider

  • src/node/utils/util.ts:64 — TSDoc summary for getAppDir used lowercase "platform.bible folder"; the brand name is "Platform.Bible" (title-case). The constant's own TSDoc correctly used "Platform.Bible". (fixed during review: changed "platform.bible folder" to "Platform.Bible folder" in the TSDoc summary line)

  • src/node/utils/util.test.ts:4 — Static top-level import of PRODUCT_FOLDER_NAME was a mild inconsistency with the vi.resetModules() + dynamic import() pattern used in the test file. (fixed during review: removed the static import and inlined '.platform.bible' in the test assertion to stay fully consistent with the existing pattern)

[Both minor findings were fixed during the review interview. No open findings remain.]

Template Propagation

Shared Regions Modified

None.

Extension Config Changes

None.

Positive Observations

  • Extracting PRODUCT_FOLDER_NAME as a named constant eliminates duplicated magic string literals and makes a future folder-name change a one-line update.
  • The stale {{ productInfo.name }} template placeholder in the getAppDir TSDoc was correctly replaced with the actual resolved value — good documentation accuracy fix.
  • papi.d.ts was updated consistently with the new export, keeping the public type surface in sync.
  • The change has no behavioral impact on getAppDir — the produced path was already .platform.bible, so this is a zero-risk refactor.
  • The original path.join(os.homedir(), '/.platform.bible') had a redundant leading / on the second argument; the template literal form using the constant removes this minor inconsistency.

Interview Notes

Author's stated purpose: part of a larger effort to correctly identify and use the right application data path depending on whether Platform.Bible is running in development or production. No Critical or Important findings were raised. Two minor findings (TSDoc capitalization, test import consistency) were fixed during the review interview. The author had no additional context to add.

In-Review Quality Check

Two files were modified during the review interview (src/node/utils/util.ts, src/node/utils/util.test.ts).

  • typecheck: Failed — pre-existing failures in lib/platform-bible-react/dist/index.d.ts and @types/react, unrelated to these changes. Zero new errors in the changed files.
  • lint: Failed at the build:types prerequisite step — same pre-existing root cause. No errors in either changed file when linting scripts directly.
  • format: Passed — no formatting changes needed.
  • tests (src/node/utils/util.test.ts): Both tests pass:
    • getAppDir > returns .platform.bible in home directory when packaged
    • getAppDir > returns resourcesPath/dev-appdata when not packaged

No auto-fixes were applied. The pre-existing typecheck/lint failures are not related to this branch.

Suggested Review Focus

  • Confirm papi.d.ts was regenerated via build:types rather than hand-edited (the diff is consistent with regeneration, but worth verifying the generation pipeline was run).
  • Confirm this is the right place to export PRODUCT_FOLDER_NAME from, given the larger effort around app data path management — or whether it should eventually live in a more prominent shared location.

This change is Reviewable

katherinejensen00 and others added 2 commits May 15, 2026 12:40
- Capitalize brand name in getAppDir TSDoc ("Platform.Bible folder")
- Remove static PRODUCT_FOLDER_NAME import from util.test.ts; inline literal string to stay consistent with vi.resetModules() pattern

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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