Skip to content

fix(frontend): remove un-needed ? in checks#239

Open
peterfortuin wants to merge 9 commits intogrimmory-tools:developfrom
peterfortuin:fix/frontend-build-warnings
Open

fix(frontend): remove un-needed ? in checks#239
peterfortuin wants to merge 9 commits intogrimmory-tools:developfrom
peterfortuin:fix/frontend-build-warnings

Conversation

@peterfortuin
Copy link
Copy Markdown
Contributor

@peterfortuin peterfortuin commented Mar 27, 2026

Description

To solve build warnings in the frontend I removed unneeded ? in checks.

Linked Issue: No linked issue.

Changes

This pull request refactors how user permissions are accessed in the app.topbar.component.html file. The main change is the removal of optional chaining (?.) when checking properties on the permissions object, assuming that permissions will always be defined on the currentUser object. This results in cleaner and slightly more efficient code.

Template cleanup and consistency:

  • Replaced all instances of currentUser.permissions?.<permission> with currentUser.permissions.<permission> throughout the topbar and mobile menu UI logic, ensuring permissions are accessed directly without optional chaining.

Summary by CodeRabbit

  • Chores

    • Adjusted CI build warning threshold used by the pipeline’s quality gate.
  • Refactor

    • Simplified topbar permission checks in navigation templates to use direct property access.
    • Removed an unused documentation-link component import from Global Preferences.

@peterfortuin peterfortuin marked this pull request as draft March 27, 2026 15:38
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9d9b9902-7711-455c-b26c-1534a0940631

📥 Commits

Reviewing files that changed from the base of the PR and between 51425c6 and 281989b.

📒 Files selected for processing (2)
  • .github/workflows/angular-lint-threshold.yml
  • frontend/src/app/features/settings/global-preferences/global-preferences.component.ts
💤 Files with no reviewable changes (1)
  • frontend/src/app/features/settings/global-preferences/global-preferences.component.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/angular-lint-threshold.yml
📜 Recent 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). (2)
  • GitHub Check: Test Suite / Backend Tests
  • GitHub Check: Analyze (java-kotlin)

📝 Walkthrough

Walkthrough

Lowered the CI build warning threshold from 24 to 0, removed optional chaining on currentUser.permissions in the topbar template, and removed ExternalDocLinkComponent from the standalone imports of the global-preferences component.

Changes

Cohort / File(s) Summary
CI Build Threshold
​.github/workflows/angular-lint-threshold.yml
Updated env BUILD_WARNING_THRESHOLD from 240, causing the lint/build-warning gate to fail the job when any warnings are present.
Frontend Topbar Template
frontend/src/app/shared/layout/component/layout-topbar/app.topbar.component.html
Replaced optional chaining checks (currentUser.permissions?.<flag>, !currentUser.permissions?.demoUser) with direct property access (currentUser.permissions.<flag>, !currentUser.permissions.demoUser) across desktop and mobile conditionals — removes null-safety for permissions.
Global Preferences Component
frontend/src/app/features/settings/global-preferences/global-preferences.component.ts
Removed ExternalDocLinkComponent import and dropped it from the component's standalone imports array. No public API changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • balazs-szucs

Poem

🐰 I tightened the gate where warnings slept,
Permissions unchained, a gamble lept.
I nibble imports, tidy and neat,
Hop careful on branches, light on my feet.
🌿✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The PR title follows the conventional commit format (fix(frontend): ...) and accurately describes the main change—removing optional chaining operators from permission checks.
Description check ✅ Passed The description includes required sections (Description, Linked Issue, Changes) and adequately explains the refactoring of permission access patterns, though it does not fully account for all changes in the PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

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

@peterfortuin peterfortuin marked this pull request as ready for review March 27, 2026 15:41
@imajes
Copy link
Copy Markdown
Member

imajes commented Mar 28, 2026

OK, a couple things -- lets fix the conflicts, but also i want to reframe the checks: they should not have a threshold at this point, lets reset all to zero. it's ok if the check fails because of it.

@peterfortuin
Copy link
Copy Markdown
Contributor Author

peterfortuin commented Mar 29, 2026

OK, a couple things -- lets fix the conflicts, but also i want to reframe the checks: they should not have a threshold at this point, lets reset all to zero. it's ok if the check fails because of it.

I have set the threshold to zero. There where 2 warnings left. I fixed one, but I have no idea how to fix the other one.

This is the one warning that is left:

▲ [WARNING] Module '@stomp/stompjs' used by 'node_modules/@stomp/rx-stomp/esm6/index.js' is not ESM

  CommonJS or AMD dependencies can cause optimization bailouts.
  For more information see: https://angular.dev/tools/cli/build#configuring-commonjs-dependencies

Because there is now one warning more then the threshold the build is failing.

@imajes Can you help me out with this?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants