refactor(fingerprint): extract platformToDisplayName helper to remove duplication#559
refactor(fingerprint): extract platformToDisplayName helper to remove duplication#559
Conversation
… duplication Co-Authored-By: Giulio Vaccari <io@giuliovaccari.it>
|
Caution Review failedPull request was closed or merged during review WalkthroughThis change refactors the platform-to-display-name mapping in Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR extracts a Confidence Score: 4/5Safe to merge; findings are P2 style/clarity issues with no functional regression. Only P2 findings: an unused type and a documented pre-existing fallback now surfaced by the centralisation. No new bugs introduced. src/plugin/fingerprint.ts — unused PlatformChoice type and Linux→MACOS silent fallback worth clarifying. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[generateFingerprint] -->|randomFrom PLATFORM_CHOICES| B["platform: 'darwin' | 'win32'"]
C[collectCurrentFingerprint] -->|os.platform| D["platform: NodeJS.Platform\n(darwin/win32/linux/...)"]
B --> E[platformToDisplayName]
D --> E
E -->|=== 'win32'| F["'WINDOWS'"]
E -->|anything else| G["'MACOS' ⚠️ Linux also maps here"]
F --> H[clientMetadata.platform]
G --> H
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/plugin/fingerprint.ts
Line: 74
Comment:
**Unused type declaration**
`PlatformChoice` is defined but never referenced anywhere in the file. `platformToDisplayName` accepts `string` rather than `PlatformChoice`, and nothing else uses this type. It's dead code that should be removed.
```suggestion
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/plugin/fingerprint.ts
Line: 80-82
Comment:
**Linux platform silently maps to MACOS**
`collectCurrentFingerprint` passes the real `os.platform()` result, which can be `"linux"` (or any other platform). The helper maps every non-`win32` value to `"MACOS"`, so Linux hosts will report `platform: "MACOS"` in the fingerprint. This logic was already present before the refactor, but now it's centralised — making it a good time to add an explicit guard or a comment for `"linux"`.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "refactor(fingerprint): extract platformT..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| const PLATFORM_CHOICES = ["darwin", "win32"] as const; | ||
| type PlatformChoice = typeof PLATFORM_CHOICES[number]; |
There was a problem hiding this comment.
PlatformChoice is defined but never referenced anywhere in the file. platformToDisplayName accepts string rather than PlatformChoice, and nothing else uses this type. It's dead code that should be removed.
| type PlatformChoice = typeof PLATFORM_CHOICES[number]; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin/fingerprint.ts
Line: 74
Comment:
**Unused type declaration**
`PlatformChoice` is defined but never referenced anywhere in the file. `platformToDisplayName` accepts `string` rather than `PlatformChoice`, and nothing else uses this type. It's dead code that should be removed.
```suggestion
```
How can I resolve this? If you propose a fix, please make it concise.| function platformToDisplayName(platform: string): "WINDOWS" | "MACOS" { | ||
| return platform === "win32" ? "WINDOWS" : "MACOS"; | ||
| } |
There was a problem hiding this comment.
collectCurrentFingerprint passes the real os.platform() result, which can be "linux" (or any other platform). The helper maps every non-win32 value to "MACOS", so Linux hosts will report platform: "MACOS" in the fingerprint. This logic was already present before the refactor, but now it's centralised — making it a good time to add an explicit guard or a comment for "linux".
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin/fingerprint.ts
Line: 80-82
Comment:
**Linux platform silently maps to MACOS**
`collectCurrentFingerprint` passes the real `os.platform()` result, which can be `"linux"` (or any other platform). The helper maps every non-`win32` value to `"MACOS"`, so Linux hosts will report `platform: "MACOS"` in the fingerprint. This logic was already present before the refactor, but now it's centralised — making it a good time to add an explicit guard or a comment for `"linux"`.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
No description provided.