Skip to content

lightning server starter questions#1

Open
levinielson wants to merge 1 commit into
masterfrom
ln/lsStarterQuestions
Open

lightning server starter questions#1
levinielson wants to merge 1 commit into
masterfrom
ln/lsStarterQuestions

Conversation

@levinielson

Copy link
Copy Markdown

No description provided.

@UnknownJoe796 UnknownJoe796 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated Review (commit 3b67407)## PR #1: lightning server starter questions — Review

Summary: This PR adds ~15 question/note comments throughout the server code and one minor type annotation. No functional code changes.


1. Question comments should not be merged to production

Files: AuthenticationEndpoints.kt, FcmTokenEndpoints.kt, Main.kt, Server.kt, deployments.kt (all added comment lines)

Issue: The PR consists almost entirely of inline question comments like // what is this?, // an explanation of this would be nice, // not sure what this is. These are developer learning notes, not documentation. Merging them pollutes the codebase with unanswered questions and TODO-like noise that will likely never be cleaned up.

Suggested fix: Move these questions to a separate document (e.g., a GitHub issue, a QUESTIONS.md, or a discussion thread) and answer them there. If the answers lead to useful documentation, add proper doc comments to the code afterward. Do not merge raw questions as code comments.

Confidence: High


2. Incomplete comment — sentence cut off

File: AuthenticationEndpoints.kt:173

// why aren't they included in the higher

This comment is truncated mid-sentence. If this were to stay (it shouldn't per issue #1), it would be confusing to future readers.

Confidence: High


3. Pre-existing: debug = true in deployment config

File: deployments.kt:46

override val debug = true

Not introduced by this PR, but the added comment // why use these in code vs as settings? draws attention to it. Having debug = true hardcoded in a deployment object that targets a real domain (api.lskiteuistarter.cs.lightningkite.com) is worth verifying — is this intentional for a staging environment, or should it be false for production?

Confidence: Medium (pre-existing, but worth flagging since it was called out)


4. Pre-existing: hardcoded UUID for auth bypass

File: AuthenticationEndpoints.kt:184

if (subject._id.toString() == "f00ffbaa-abf9-497d-a75a-442f1c77c1e9") return 10

Again not introduced by this PR, but the question comment highlights it. This hardcodes a specific user ID to receive reduced proof strength (10 instead of potentially 20). The existing // AppStoreTester comment explains the intent, but a magic UUID with weakened auth requirements in production code deserves a more thorough explanation or should be behind a config flag.

Confidence: Medium (pre-existing)


No issues with the actual code change

The only functional change — adding an explicit type annotation in UserEndpoints.kt:121 (val info: ModelInfo<User, User, Uuid>) — is harmless and arguably improves readability.


Bottom line: This PR should not be merged as-is. The questions are valid and worth answering, but they belong in a discussion or issue, not as inline comments in production code.

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.

2 participants