fix(metadata): hide sidecar tab when sidecar json is disabled by zachyale · Pull Request #814 · grimmory-tools/grimmory

zachyale · 2026-04-23T20:55:43Z

Description

Currently, Sidecar tab is gated by user permissions, is physical and is local, but not if Sidecar is enabled or not. Hide the Sidecar tab in the book details metadata center when Enable Sidecar JSON is disabled.

Linked Issue: Fixes #810

Changes

hide sidecar tab visibility on existing sidecar logic && [...].sidecarSettings.enabled

Note: In testing this, I found that setting the URL param for the view to a view a user doesn't have permissions for just shows an empty screen. Created a follow up outside of this PR on #813

Summary by CodeRabbit

Refactor
- Improved sidecar metadata tab visibility controls using configuration-based logic.

coderabbitai · 2026-04-23T20:55:54Z

📝 Walkthrough

Walkthrough

The changes introduce a computed canShowSidecarTab getter to control sidecar tab visibility in book metadata view, based on settings and user permissions. The template now uses this single boolean instead of complex conditional logic. Tests verify the visibility toggle when sidecar settings are enabled or disabled.

Changes

Cohort / File(s)	Summary
Sidecar Tab Visibility Control `frontend/src/app/features/metadata/component/book-metadata-center/book-metadata-center.component.html`, `book-metadata-center.component.ts`, `book-metadata-center.component.spec.ts`	Adds `canShowSidecarTab` computed getter combining sidecar settings enabled state with user permissions (`admin` or `canEditMetadata`) and environment constraints (`!isPhysical` and `isLocalStorage`). Template conditional simplified to use new getter. Comprehensive unit tests verify visibility behavior when sidecar settings toggle.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

frontend, enhancement

Suggested reviewers

imajes
balazs-szucs

Poem

🐰 A curious rabbit hopped through the code,
And found where the sidecar tab strode,
Now settings control what users can see,
When JSON's disabled, the tab won't be free! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title follows conventional commit format with fix type and descriptive scope about hiding sidecar tab based on settings.
Description check	✅ Passed	The description includes problem statement, linked issue, and specific implementation details about the changes required.
Linked Issues check	✅ Passed	Changes implement the requirement from `#810` by adding conditional logic to hide sidecar tab when sidecar JSON setting is disabled.
Out of Scope Changes check	✅ Passed	All changes are scoped to the sidecar tab visibility logic and related tests; no unrelated modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/disable-sidecar-tab-via-settings

✨ Simplify code

Create PR with simplified code
Commit simplified code in branch fix/disable-sidecar-tab-via-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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

🧹 Nitpick comments (2)

frontend/src/app/features/metadata/component/book-metadata-center/book-metadata-center.component.ts (1)
89-94: Consider a computed signal for reactivity.

canShowSidecarTab is a plain getter that reads a signal (appSettings()) plus plain fields (admin, canEditMetadata, isLocalStorage). It works today because the template re-evaluates on change detection, but expressing it as a computed() would make it properly reactive to settings changes and align with the team's signal-first pattern—especially if admin/canEditMetadata/isLocalStorage are ever migrated to signals (note isLocalStorage is currently only assigned once in ngOnInit, so it won't react to later diskType changes).

Based on learnings: prefer Angular signals (signal/computed) over imperative patterns where they fit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@frontend/src/app/features/metadata/component/book-metadata-center/book-metadata-center.component.ts`
around lines 89 - 94, Replace the plain getter canShowSidecarTab with an Angular
computed signal so its value updates reactively when appSettings() or any future
signals (admin, canEditMetadata, isLocalStorage) change: create a computed that
reads this.appSettingsService.appSettings() and the component fields (admin,
canEditMetadata, isPhysical, isLocalStorage) and returns the same boolean
expression currently in the getter; update template bindings to use the computed
value and remove the getter; if isLocalStorage is set only in ngOnInit consider
converting it to a signal or updating it through a signal source so the computed
reacts to diskType changes.
frontend/src/app/features/metadata/component/book-metadata-center/book-metadata-center.component.spec.ts (1)
37-50: Test coverage is narrow — consider covering permission/physical/localStorage gates.

The suite only toggles sidecarEnabled. Since canShowSidecarTab also depends on (admin || canEditMetadata), !isPhysical, and isLocalStorage, a regression in any of those conditions would not be caught. Also note that ngOnInit is never invoked here, so isLocalStorage stays at its field default (true) and diskType in buildSettings is effectively unexercised — a future refactor that moves isLocalStorage derivation out of ngOnInit could silently change behavior without failing this spec.

Consider adding cases for: user without admin/canEditMetadata, a physical book, and non-LOCAL disk type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@frontend/src/app/features/metadata/component/book-metadata-center/book-metadata-center.component.spec.ts`
around lines 37 - 50, The test only toggles sidecarEnabled and never exercises
the other gates used by BookMetadataCenterComponent.canShowSidecarTab; update
the spec to call ngOnInit and add cases that toggle user permissions (admin and
canEditMetadata), physical/disk type variations (isPhysical/diskType in
buildSettings) and isLocalStorage to cover all branches: create instances via
TestBed.runInInjectionContext(() => new BookMetadataCenterComponent()), call
component.ngOnInit(), then set appSettingsSignal with buildSettings variations
(sidecarEnabled true/false, diskType set to physical/non-LOCAL) and adjust the
user/permission signal or input to simulate admin=false and
canEditMetadata=false to assert canShowSidecarTab false, and assert true only
when sidecarEnabled + (admin||canEditMetadata) + !isPhysical + isLocalStorage
are satisfied.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@frontend/src/app/features/metadata/component/book-metadata-center/book-metadata-center.component.spec.ts`:
- Around line 37-50: The test only toggles sidecarEnabled and never exercises
the other gates used by BookMetadataCenterComponent.canShowSidecarTab; update
the spec to call ngOnInit and add cases that toggle user permissions (admin and
canEditMetadata), physical/disk type variations (isPhysical/diskType in
buildSettings) and isLocalStorage to cover all branches: create instances via
TestBed.runInInjectionContext(() => new BookMetadataCenterComponent()), call
component.ngOnInit(), then set appSettingsSignal with buildSettings variations
(sidecarEnabled true/false, diskType set to physical/non-LOCAL) and adjust the
user/permission signal or input to simulate admin=false and
canEditMetadata=false to assert canShowSidecarTab false, and assert true only
when sidecarEnabled + (admin||canEditMetadata) + !isPhysical + isLocalStorage
are satisfied.

In
`@frontend/src/app/features/metadata/component/book-metadata-center/book-metadata-center.component.ts`:
- Around line 89-94: Replace the plain getter canShowSidecarTab with an Angular
computed signal so its value updates reactively when appSettings() or any future
signals (admin, canEditMetadata, isLocalStorage) change: create a computed that
reads this.appSettingsService.appSettings() and the component fields (admin,
canEditMetadata, isPhysical, isLocalStorage) and returns the same boolean
expression currently in the getter; update template bindings to use the computed
value and remove the getter; if isLocalStorage is set only in ngOnInit consider
converting it to a signal or updating it through a signal source so the computed
reacts to diskType changes.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3364c57e-b149-421f-9d73-b42e71caaab9

📥 Commits

Reviewing files that changed from the base of the PR and between 03389d7 and 40bb32a.

📒 Files selected for processing (3)

frontend/src/app/features/metadata/component/book-metadata-center/book-metadata-center.component.html
frontend/src/app/features/metadata/component/book-metadata-center/book-metadata-center.component.spec.ts
frontend/src/app/features/metadata/component/book-metadata-center/book-metadata-center.component.ts

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

GitHub Check: Test Suite / Frontend Tests
GitHub Check: Test Suite / Backend Tests
GitHub Check: Analyze (javascript-typescript)
GitHub Check: Analyze (java-kotlin)
GitHub Check: Frontend Lint Threshold Check

🧰 Additional context used

📓 Path-based instructions (5)

frontend/src/**/*.{ts,tsx,html,scss}