fix: correctly parse periods in share names from ini file #1629
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds two new share sections (one with periods, one with an emoji) to dev INI state, appends Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant PC as parseConfig
participant INI as parseIni
participant FPS as flattenPeriodSections
participant FOA as fixObjectArrays
participant CCK as camelCaseKeysDeep
Caller->>PC: parseConfig(rawIni)
PC->>INI: parseIni(rawIni)
INI-->>PC: parsedObj (nested for dotted sections)
PC->>FPS: flattenPeriodSections(parsedObj)
FPS-->>PC: flatObj (e.g., "system.with.periods": {...})
PC->>FOA: fixObjectArrays(flatObj)
FOA-->>PC: arraysFixed
PC->>CCK: camelCaseKeysDeep(arraysFixed)
CCK-->>PC: finalConfig
PC-->>Caller: finalConfig
note over FPS: New normalization step converts dotted INI sections into flat keys
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @pujitm's task —— View job Todo List
Critical Issue FoundSECURITY: Potential prototype pollution vulnerability in const section = {};
const nestedObjs = {};While key filtering protects against direct pollution, these containers should use Fix: Replace with: const section = Object.create(null);
const nestedObjs = Object.create(null);All other changes appear safe - test data additions and unused import removal pose no critical risks. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
api/src/__test__/store/modules/emhttp.test.ts (2)
1-6: Remove unused import flagged by CodeQLStateFileKey isn’t used; drop it to silence lint/checks.
-import { FileLoadStatus, StateFileKey } from '@app/store/types.js'; +import { FileLoadStatus } from '@app/store/types.js';
1300-1357: Lookup test covers dotted names via getShares — goodSpy is restored; minimal mocking. Consider adding afterEach(vi.restoreAllMocks) for safety if more spies are added later.
Example:
-import { beforeEach, describe, expect, test, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'; ... describe('Share parsing with periods in names', () => { beforeEach(() => { vi.clearAllMocks(); }); + afterEach(() => { + vi.restoreAllMocks(); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
api/dev/states/shares.ini(1 hunks)api/src/__test__/core/utils/shares/get-shares.test.ts(2 hunks)api/src/__test__/store/modules/emhttp.test.ts(3 hunks)api/src/__test__/store/state-parsers/shares.test.ts(1 hunks)api/src/core/utils/misc/parse-config.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/__test__/store/state-parsers/shares.test.tsapi/src/core/utils/misc/parse-config.tsapi/src/__test__/core/utils/shares/get-shares.test.tsapi/src/__test__/store/modules/emhttp.test.ts
api/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer not to mock simple dependencies in API tests
Files:
api/src/__test__/store/state-parsers/shares.test.tsapi/src/__test__/core/utils/shares/get-shares.test.tsapi/src/__test__/store/modules/emhttp.test.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
api/src/__test__/store/state-parsers/shares.test.tsapi/src/__test__/core/utils/shares/get-shares.test.tsapi/src/__test__/store/modules/emhttp.test.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/__test__/store/state-parsers/shares.test.tsapi/src/core/utils/misc/parse-config.tsapi/src/__test__/core/utils/shares/get-shares.test.tsapi/src/__test__/store/modules/emhttp.test.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/__test__/store/state-parsers/shares.test.tsapi/src/__test__/core/utils/shares/get-shares.test.tsapi/src/__test__/store/modules/emhttp.test.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
api/src/__test__/store/state-parsers/shares.test.tsapi/src/__test__/core/utils/shares/get-shares.test.tsapi/src/__test__/store/modules/emhttp.test.ts
**/__test__/store/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)
**/__test__/store/**/*.ts: For Pinia store unit tests, initialize with setActivePinia(createPinia()) rather than createTestingPinia, unless testing features from createTestingPinia are explicitly needed
Do not mock the store under test; allow the store to initialize with its natural default state
In store tests, verify action side effects and state changes, and assert that actions are called with the correct parameters
In store tests, mock external dependencies used by the store and verify interactions with those mocks
Test computed properties/getters by accessing them directly and verifying derived state
Thoroughly test async store actions: assert intermediate loading state, await completion, and check final state
Files:
api/src/__test__/store/state-parsers/shares.test.tsapi/src/__test__/store/modules/emhttp.test.ts
**/{store,stores}/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)
In store files, explicitly import Vue reactivity utilities (e.g., computed, ref, watchEffect); do not rely on Nuxt auto-imports
Files:
api/src/__test__/store/state-parsers/shares.test.tsapi/src/__test__/store/modules/emhttp.test.ts
🧠 Learnings (10)
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: This Nuxt app is tested with vitest outside the Nuxt runtime; structure tests accordingly
Applied to files:
api/src/__test__/store/modules/emhttp.test.ts
📚 Learning: 2025-08-11T15:07:39.222Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-08-11T15:07:39.222Z
Learning: Applies to api/**/*.{test,spec}.{js,jsx,ts,tsx} : Use Vitest for tests in the api; do not use Jest
Applied to files:
api/src/__test__/store/modules/emhttp.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Applied to files:
api/src/__test__/store/modules/emhttp.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Applied to files:
api/src/__test__/store/modules/emhttp.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use vi.spyOn() to specify return values or behavior of methods under test
Applied to files:
api/src/__test__/store/modules/emhttp.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to api/**/jest*.{js,ts,cjs,mjs,json} : In the API package, use Vitest for tests and do not add Jest configuration
Applied to files:
api/src/__test__/store/modules/emhttp.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Applied to files:
api/src/__test__/store/modules/emhttp.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Applied to files:
api/src/__test__/store/modules/emhttp.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/**/*.{test,spec}.{ts,js,tsx,jsx} : Store testing with Pinia: let stores initialize with natural defaults; do not mock the store under test
Applied to files:
api/src/__test__/store/modules/emhttp.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/store/**/*.ts : Thoroughly test async store actions: assert intermediate loading state, await completion, and check final state
Applied to files:
api/src/__test__/store/modules/emhttp.test.ts
🧬 Code graph analysis (1)
api/src/__test__/store/modules/emhttp.test.ts (2)
api/src/core/utils/misc/parse-config.ts (1)
parseConfig(142-199)api/src/core/utils/shares/get-shares.ts (1)
getShares(21-63)
🪛 GitHub Check: CodeQL
api/src/__test__/store/modules/emhttp.test.ts
[notice] 5-5: Unused variable, import, function or class
Unused import StateFileKey.
🔇 Additional comments (7)
api/dev/states/shares.ini (2)
68-85: Added luksStatus and new system.with.periods section — looks goodINI shape and field set match existing sections; will be correctly flattened by the updated parser.
86-102: Emoji in section name is acceptableUTF-8 content parses fine and snapshots cover it. No issues spotted.
api/src/__test__/core/utils/shares/get-shares.test.ts (2)
98-139: Snapshots updated to include period-named share — OKStructure and values align with other entries; consistent with parser behavior.
256-297: Snapshots updated to include emoji-named share — OKCovers the added INI sections; no fragility introduced.
api/src/core/utils/misc/parse-config.ts (1)
174-176: Integrating flattening into parseConfig is appropriateEnsures downstream parsers see flat sections; pipeline remains unchanged otherwise.
api/src/__test__/store/state-parsers/shares.test.ts (1)
95-132: Snapshot extensions for dotted/emoji shares — OKValidates end-to-end parsing from INI through state parser; consistent with new helper.
api/src/__test__/store/modules/emhttp.test.ts (1)
1153-1298: New parser tests for dotted names — well targetedExercise parseConfig and the three parsers; assertions focus on behavior, not internals. Nice.
pujitm
changed the title
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
🤖 I have created a release *beep* *boop* --- ## [4.16.0](v4.15.1...v4.16.0) (2025-08-27) ### Features * add `parityCheckStatus` field to `array` query ([#1611](#1611)) ([c508366](c508366)) * generated UI API key management + OAuth-like API Key Flows ([#1609](#1609)) ([674323f](674323f)) ### Bug Fixes * **connect:** clear `wanport` upon disabling remote access ([#1624](#1624)) ([9df6a3f](9df6a3f)) * **connect:** valid LAN FQDN while remote access is enabled ([#1625](#1625)) ([aa58888](aa58888)) * correctly parse periods in share names from ini file ([#1629](#1629)) ([7d67a40](7d67a40)) * **rc.unraid-api:** remove profile sourcing ([#1622](#1622)) ([6947b5d](6947b5d)) * remove unused api key calls ([#1628](#1628)) ([9cd0d6a](9cd0d6a)) * retry VMs init for up to 2 min ([#1612](#1612)) ([b2e7801](b2e7801)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Share names live as section headers in
emhttp/state/shares.ini. However, periods in ini section headers typically denote nested hierarchy. This behavior is disabled in unraid's php setup, but cannot be disabled/configured in the api's current ini parser.So, we perform post-processing to reconcile nested objects into dot notation.
Known issue: trailing and leading periods will not be treated as distinct from shares without them.
i.e.
.share.name.will conflict withshare.name,.share.name, orshare.name.. The last of the conflicting names will be used/exposed.Summary by CodeRabbit
New Features
Bug Fixes
Tests